diff --git a/conductor/code_styleguides/error_handling.md b/conductor/code_styleguides/error_handling.md index d124e823..9f2bcd3b 100644 --- a/conductor/code_styleguides/error_handling.md +++ b/conductor/code_styleguides/error_handling.md @@ -353,6 +353,170 @@ HTTP status code is the framework contract. --- +## Drain Points: Where Result[T] Propagation Terminates + +A `Result[T]` returned from a function that can fail at runtime +**propagates upward through the call stack** until it reaches a **drain +point** — a place where the error is HANDLED visibly to the user or via +intentional app action. The drain point is the END of the propagation. + +The user's principle (2026-06-17): + +> "IF ANY PLACE HAS A ERROR LOG IT ALSO NEEDS A RESULT[T]. RESULT[T] +> PROPOGATES UNTIL IT REACHED A 'DRAIN' POINT WHERE THE ERROR CAN BE +> HANDLED APPROPRIATELY WITHOUT CRASHING THE APP. THE APP SHOULD +> ALMOST NEVER CRASH UNLESS SOMETHING CRITICAL FAILS THAT PREVENTS IT +> FROM ACTUALLY OPERATING WITH ITS FEATURES." + +A drain point is **not** an excuse to swallow the error. It is the +place where the error is INTENTIONALLY resolved (displayed to the user, +recorded in telemetry, or used to drive an app-level decision) — and +where the caller of the drain point does NOT need to receive a +`Result[T]` back. + +### The 5 drain point patterns + +**Pattern 1 — HTTP error response (in `_api_*` FastAPI handler):** + +```python +# COMPLIANT: drain point. The HTTP status code IS the error response. +async def _api_get_track(controller, track_id: str) -> dict: + result = controller.get_track_result(track_id) + if not result.ok: + raise HTTPException(status_code=404, detail=result.errors[0].ui_message()) + return {"track": result.data} +``` + +The caller (the HTTP client) receives an HTTP 4xx/5xx response. The +error has been "drained" — the controller doesn't return a `Result[T]` +to its caller; it raises into the FastAPI framework, which serializes +the error. + +**Pattern 2 — GUI error display:** + +```python +# COMPLIANT: drain point. The user sees the error in the modal. +def _show_track_load_failure(controller, track_id: str) -> None: + result = controller.get_track_result(track_id) + if not result.ok: + imgui.open_popup("Track Load Error") + # popup body reads result.errors[0].ui_message() and displays it +``` + +The user sees the error. The caller (`_show_track_load_failure`) +returns `None` — it is the end of the propagation chain. + +**Pattern 3 — Intentional app termination:** + +```python +# COMPLIANT: drain point. The app shuts down intentionally. +def _shutdown_on_critical_failure(controller) -> None: + result = controller._init_session_db_result() + if not result.ok: + sys.stderr.write(f"FATAL: {result.errors[0].ui_message()}\n") + sys.exit(1) +``` + +The error is propagated to the OS via `sys.exit(1)`. The drain point +is the process termination itself. + +**Pattern 4 — Telemetry emission:** + +```python +# COMPLIANT: drain point. The error is sent to monitoring. +def _report_failure_to_telemetry(controller, op_name: str, result: Result[T]) -> None: + if not result.ok: + telemetry.emit_error( + operation=op_name, + kind=result.errors[0].kind.value, + message=result.errors[0].message, + ) +``` + +The error reaches the telemetry system. The caller of the drain point +receives `None`. + +**Pattern 5 — Retry-with-bounded-attempts:** + +```python +# COMPLIANT: drain point. The retry is bounded and the final failure +# is reported back to the user (which is itself a drain point). +def _load_track_with_retry(controller, track_id: str) -> Track | None: + for attempt in range(MAX_RETRIES): + result = controller.get_track_result(track_id) + if result.ok: + return result.data + time.sleep(BACKOFF_SECONDS * (attempt + 1)) + return None # Caller will display "failed after N attempts" +``` + +The retry loop is a drain point: the function returns `Track | None` +because the caller (a GUI function) handles `None` by showing a +"failed after N attempts" message. The retry is bounded (no infinite +loops); the final `None` propagates to a visible error UI. + +### What is NOT a drain point + +The following are **NOT** drain points. They are silent-fallback +violations that lose data: + +- **`sys.stderr.write(...)` alone** (without visible user feedback or + app-level decision): the data is lost; the user sees nothing. + Logging is NOT a drain. +- **`logging.error(...)` / `logger.exception(...)` alone**: same as + above. The log is recorded, but the error is invisible to the user. +- **`return default_value`** after a `try/except`: the original error + context is lost; the caller cannot distinguish success from failure. +- **`pass`**: silent. The data is lost. +- **`traceback.print_exc(...)` alone**: similar to logging — visible in + the console but invisible to the user. + +**The key distinction:** a drain point **terminates the propagation** +with a visible, intentional action. A log call or silent fallback +**discards the error** without terminating the propagation. + +### Boundary types vs. drain points + +The two concepts are complementary: + +- **Boundary types** (Section: "Boundary Types") describe WHERE + exceptions originate or are converted (third-party SDK calls, stdlib + I/O, FastAPI handlers). The catch site at a boundary converts the + exception to `ErrorInfo` and returns it in `Result`. +- **Drain points** describe WHERE the `Result[T]` propagation + terminates (HTTP error response, GUI display, app termination, + telemetry, bounded retry). The function at a drain point returns + `None` or raises into a framework; it does NOT return `Result[T]`. + +A function can be BOTH a boundary AND a drain point. The +`_api_*` FastAPI handler is a boundary (catches SDK exceptions) and a +drain point (raises HTTPException, terminating the propagation). +Audit heuristic `BOUNDARY_FASTAPI` covers both aspects. + +### Audit heuristic Heuristic D + +The audit script (`scripts/audit_exception_handling.py`) has a +Heuristic D that recognizes drain-point patterns as `INTERNAL_COMPLIANT`. +The patterns are: + +1. `except (SomeError): self.send_response(status); ...` (HTTP + response in a `BaseHTTPRequestHandler` subclass) +2. `except (SomeError): imgui.open_popup(...)` (GUI error display) +3. `except (SomeError): sys.exit(...)` (intentional termination) +4. `except (SomeError): telemetry.emit_*(...)` (telemetry) +5. `except (SomeError): for attempt in range(N): ...; return None` + (bounded retry; followed by `return None` or similar end-of-propagation) + +A site matching any of these is classified `INTERNAL_COMPLIANT`, with a +note that the pattern is a drain point. + +A site that calls `sys.stderr.write(...)` or `logging.error(...)` in +the except body is **NOT** matched by Heuristic D — those are not +drain points per the user's principle. They are flagged as +`INTERNAL_SILENT_SWALLOW` (a violation). + +--- + ## The Broad-Except Distinction Anti-pattern #6 says "DON'T catch `except Exception` and silently swallow." @@ -362,11 +526,17 @@ But `except Exception` is **not always a violation**. The distinction is | What the catch does | Classification | Convention status | |---|---|---| | `pass` (or no body) | `INTERNAL_SILENT_SWALLOW` | **Violation** | -| `print(...)` / `log(...)` only | `INTERNAL_SILENT_SWALLOW` | **Violation** (the data is lost) | +| `print(...)` / `log(...)` only (broad catch + log) | `INTERNAL_SILENT_SWALLOW` | **Violation** (the data is lost) | +| `narrow except + log only` (e.g., `except (OSError, ValueError): sys.stderr.write(...)`) | `INTERNAL_SILENT_SWALLOW` | **Violation** — **logging is NOT a drain**. The user's principle (2026-06-17) explicitly states: `sys.stderr.write` / `logging.error` / `logger.exception` / `traceback.print_exc` alone is NOT a drain point. The error context is lost. Use `Result[T]` propagation and let the error reach a true drain point. | | `return None` / `return Optional[T]` | `INTERNAL_OPTIONAL_RETURN` | **Violation** (use `Result[T]`) | | `return Result(data=..., errors=[ErrorInfo(...)])` | `BOUNDARY_CONVERSION` | **Compliant** (the canonical pattern) | | `raise` (re-raise) | `INTERNAL_RETHROW` (or `BOUNDARY_SDK` if at third-party call) | **Suspicious** (often refactorable) | | `raise HTTPException(...)` (in `_api_*` handler) | `BOUNDARY_FASTAPI` | **Compliant** (the framework contract) | +| HTTP error response (drain point) | `INTERNAL_COMPLIANT` (Heuristic D) | **Compliant** (the propagation terminates with visible user feedback) | +| GUI error display (drain point) | `INTERNAL_COMPLIANT` (Heuristic D) | **Compliant** | +| Intentional app termination (drain point) | `INTERNAL_COMPLIANT` (Heuristic D) | **Compliant** | +| Telemetry emission (drain point) | `INTERNAL_COMPLIANT` (Heuristic D) | **Compliant** | +| Bounded retry (drain point) | `INTERNAL_COMPLIANT` (Heuristic D) | **Compliant** | **The canonical pattern** (in `_result` functions that wrap third-party SDK calls): @@ -644,6 +814,31 @@ Exception`, etc.) which is the OPPOSITE of this convention. The checklist below catches the most common LLM mistakes. **Run this checklist before claiming a task is done.** +### Rule #0 — READ THIS STYLEGUIDE FIRST (Added 2026-06-17) + +**Before writing or modifying ANY `try/except` code, you MUST:** + +1. **READ `conductor/code_styleguides/error_handling.md` end-to-end.** + The 7 sections are: (1) The 5 Patterns, (2) Decision Tree, + (3) Anti-Patterns, (4) Hard Rules, (5) Boundary Types, (6) The + Broad-Except Distinction, (7) AI Agent Checklist (this section). + +2. **Acknowledge the read in the commit message.** Format: "TIER-2 + READ conductor/code_styleguides/error_handling.md before + ." + +3. **The styleguide is the source of truth.** Your training data is + the OPPOSITE of this convention. Idiomatic Python (`try/except` + + `Optional[T]` + `raise Exception`) is what the convention is + designed to REPLACE. + +**Why:** the previous round (Phase 10) added 5 LAUNDERING HEURISTICS to +the audit script that classified narrowing as compliant, which is the +OPPOSITE of what the styleguide says. The agent had not read the +styleguide end-to-end and re-derived a permissive rule from training +data. **Reading the styleguide is the explicit defense against +re-introducing laundering heuristics.** + ### The 5 MUST-DO rules When writing NEW code, you MUST: