Private
Public Access
18 KiB
18 KiB
Result Migration Sub-Track 1: Review Pass Report
Track: result_migration_review_pass_20260617
Umbrella: result_migration_20260616
Type: audit + documentation (informational; no production code change)
Status: active
Date: 2026-06-17
0. Executive Summary
This report captures the per-site decisions for the 43 ambiguous exception-handling sites identified by scripts/audit_exception_handling.py --json on 2026-06-17:
- 24 UNCLEAR sites (the script cannot classify from AST alone)
- 19 INTERNAL_RETHROW sites (
try/except + raise; needs the 3 legitimate pattern checks)
Each site was reviewed by reading the snippet + 2-3 lines of context. The decisions flow into the umbrella's sub-tracks 2-4 as their starting migration scope.
1. Pre-Review Audit Snapshot (2026-06-17, base commit b6caca40)
| Bucket | Count | Description |
|---|---|---|
UNCLEAR |
24 | Script could not classify; needs human review |
INTERNAL_RETHROW |
19 | try/except + raise; needs 3-pattern check |
| Total review scope | 43 | 11 files affected |
Other audit findings (unchanged by this review pass):
- 211 violations (broad catch, silent swallow, Optional[T] return) — out of scope here
- 80 compliant sites — out of scope here
- 25 INTERNAL_PROGRAMMER_RAISE (raise in init / assert) — compliant; out of scope
2. Per-Site Decision Table
2.1 src/gui_2.py — UNCLEAR sites (13)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 65 | _resolve (deferred importer) |
except AttributeError: ... _FiledialogStub() |
compliant | Graceful degradation for missing optional modules (filedialog stub) |
| 69 | _resolve (deferred importer) |
except (ImportError, ModuleNotFoundError): _FiledialogStub() |
compliant | Graceful degradation for missing optional modules (filedialog stub) |
| 684 | run (ImGui main loop) |
except RuntimeError as _immapp_exc: ... log + keep alive |
compliant | Defer-not-catch for native bundle crashes (per workflow.md); logs to _gui_degraded_reason |
| 806 | _get_active_capabilities |
except KeyError: caps = VendorCapabilities(... notes="unregistered") |
compliant | Lookup-miss-with-default for get_capabilities(provider, model) |
| 1349 | _populate_auto_slices |
except Exception: return |
migration-target | Broad except Exception + silent return. Should narrow to (OSError, UnicodeDecodeError) or return Result. Sub-track 4 (gui_2) |
| 2401 | render_rag_panel (vector store provider combo) |
except (ValueError, AttributeError): idx = 0 |
compliant | list.index miss with default; standard Python combo-box idiom |
| 2411 | render_rag_panel (embedding provider combo) |
except (ValueError, AttributeError): idx_e = 0 |
compliant | list.index miss with default; standard Python combo-box idiom |
| 2533 | render_agent_tools_panel (tool preset combo) |
except ValueError: idx = 0 |
compliant | list.index miss with default; standard Python combo-box idiom |
| 2561 | render_agent_tools_panel (filter category combo) |
except ValueError: f_idx = 0 |
compliant | list.index miss with default; standard Python combo-box idiom |
| 2759 | render_persona_selector_panel (load persona context preset) |
except KeyError as e: app.ai_status = f"persona context preset missing: {e}" |
compliant | Lookup-miss-with-user-feedback; defensive but user-visible |
| 4106 | render_context_files_table (view mode combo) |
except ValueError: current_idx = 1; f_item.view_mode = "summary" |
compliant | list.index miss with default + state correction |
| 4159 | render_context_presets (context preset combo) |
except ValueError: idx = 0 |
compliant | list.index miss with default; standard Python combo-box idiom |
| 6830 | render_tier_stream_panel (ImGui child end guard) |
except (TypeError, AttributeError): imgui.end_child() |
compliant | ImGui scope cleanup guard; ensures end_child() is always called |
Subtotals: 12 compliant + 1 migration-target.
New heuristics identified for the audit script (added in Task 4.1):
list.indexwithValueErrorfallback to a default index →INTERNAL_COMPLIANTdict.get/KeyErrorlookup with default value construction →INTERNAL_COMPLIANT- Narrow
except (RuntimeError, OSError, AttributeError, ImportError)+imgui.end_*or stub construction →INTERNAL_COMPLIANT(defer-not-catch for ImGui) - Narrow
except (ImportError, ModuleNotFoundError, AttributeError)+ fallback attribute/stub →INTERNAL_COMPLIANT(graceful degradation)
2.2 src/mcp_client.py — UNCLEAR sites (4, baseline)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 126 | configure (allowlist setup) |
except (OSError, ValueError): rp = Path(p).resolve() (non-strict fallback) |
compliant | Graceful path resolution: Path.resolve(strict=True) may fail if file missing; fallback to non-strict is a safe degradation |
| 152 | _is_allowed (allowlist check) |
except (OSError, ValueError): rp = path.resolve() (non-strict fallback) |
compliant | Graceful path resolution (same as L126) |
| 177 | _is_allowed (cwd subpath check) |
except ValueError: pass after rp.relative_to(cwd) |
compliant | Path.relative_to raises ValueError when path is not relative to base; this is the canonical "not-a-subpath" check, not an error |
| 987 | py_check_syntax (tool function) |
except SyntaxError: ... then except Exception: return f"ERROR..." |
compliant | Tool-boundary pattern: function returns a string (Result-like); both narrow and broad excepts convert exceptions to user-readable strings. No silent swallow |
Subtotals: 4 compliant + 0 migration-target.
New heuristic candidates:
5. Path.resolve(strict=True) with (OSError, ValueError) fallback to non-strict → INTERNAL_COMPLIANT (graceful path resolution)
6. Path.relative_to with ValueError (not-a-subpath) → INTERNAL_COMPLIANT (canonical subpath check)
7. MCP tool function with except Exception: return f"ERROR..." (string return) → BOUNDARY_TOOL (tool boundary; converts to string Result)
2.3 src/ai_client.py — UNCLEAR sites (2, baseline)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 828 | run_with_tool_loop (sync/async bridge) |
except RuntimeError: results = asyncio.run(...) after asyncio.get_running_loop() |
compliant | Sync/async bridge: get_running_loop() raises RuntimeError when no loop is running; the fallback to asyncio.run is the canonical pattern |
| 2813 | _get_llama_cost_tracking (vendor capabilities lookup) |
except KeyError: return True after get_capabilities("llama", _model) |
compliant | Lookup-miss-with-default (same as gui_2 L806); default to cost-tracking-on for unknown models |
Subtotals: 2 compliant + 0 migration-target.
New heuristic candidates:
8. asyncio.get_running_loop() with except RuntimeError: asyncio.run(...) → INTERNAL_COMPLIANT (sync/async bridge)
2.4 src/app_controller.py — UNCLEAR sites (2)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 1842 | init_state (controller initialization) |
except KeyError: caps = None after get_capabilities(...) |
compliant | Lookup-miss-with-None default; same pattern as L806/L2813; downstream check if caps is None or caps.model_discovery |
| 3740 | _on_ai_stream (streaming handler) |
except KeyError: caps = None after get_capabilities(...) |
compliant | Lookup-miss-with-None default; downstream check if caps is None or caps.streaming |
Subtotals: 2 compliant + 0 migration-target.
2.5 src/models.py — UNCLEAR sites (2)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 452 | from_dict (track-state deserialization) |
except ValueError: created = None after datetime.fromisoformat(created) |
compliant | Lenient deserialization: malformed ISO date in TOML config → None (don't crash the entire load). Canonical pattern for user-edited config |
| 457 | from_dict (track-state deserialization) |
except ValueError: updated = None after datetime.fromisoformat(updated) |
compliant | Lenient deserialization (same as L452) |
Subtotals: 2 compliant + 0 migration-target.
New heuristic candidates:
9. datetime.fromisoformat(s) with except ValueError: <var> = None → INTERNAL_COMPLIANT (lenient TOML deserialization)
2.6 src/multi_agent_conductor.py — UNCLEAR sites (1)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 236 | parse_json_tickets (CLI-style JSON input) |
except json.JSONDecodeError as e: print(...); except KeyError as e: print(...) |
compliant | CLI-style input parser: print provides user-visible error feedback; the function is -> None so there is no Result to add. The narrow excepts are appropriate for the two distinct failure modes (malformed JSON vs missing required field) |
Subtotals: 1 compliant + 0 migration-target.
New heuristic candidates:
10. try/except (json.JSONDecodeError, KeyError) around JSON parse with print(...) and return (no Result) → INTERNAL_COMPLIANT (CLI-style JSON input parser)
2.7 src/ai_client.py — INTERNAL_RETHROW sites (6, baseline)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 277 | _load_credentials (file load) |
except FileNotFoundError: raise FileNotFoundError(...) with helpful setup message |
PATTERN_1 | Catch + convert + raise as same type with better message. Provides actionable instructions in the error message. Baseline transition pattern. |
| 801 | _default_send (Result→Exception bridge) |
if not res.ok: ... raise res.errors[0].original |
PATTERN_1 | Result→Exception bridge: re-raise original SDK exception. Legacy callers expect exceptions; the Result layer above provides the structured error info |
| 802 | _default_send (Result→Exception bridge) |
raise RuntimeError(res.errors[0].message if res.errors else "Unknown OpenAI error") |
PATTERN_1 | Result→Exception bridge: convert Result error to RuntimeError. Same as L801 |
| 1234 | _list_anthropic_models (Anthropic SDK) |
except Exception as exc: raise _classify_anthropic_error(exc) from exc |
PATTERN_1 | Catch + convert + raise as different type: convert raw SDK exception to structured ErrorInfo. from exc preserves the traceback |
| 1529 | _list_gemini_models (Gemini SDK) |
except Exception as exc: raise _classify_gemini_error(exc) from exc |
PATTERN_1 | Same as L1234, Gemini SDK |
| 2520 | _dashscope_call (Qwen/DashScope SDK) |
if status_code != 200: raise classify_dashscope_error(...) |
PATTERN_1 | Result→Exception bridge: explicit raise on API non-200 status. Caller (Result-based) catches and converts. No try/except in this function; the raise is the explicit "this is a domain error" path |
Subtotals: 6 PATTERN_1 + 0 PATTERN_2/3 + 0 migration-target.
Note: All 6 baseline ai_client INTERNAL_RETHROW sites are the "Result→Exception bridge" pattern. This is the canonical pattern for the baseline transition: Result-based provider functions still raise on hard failures for legacy callers, but the convention layer above catches and converts to a Result. The 2026-06-12 refactor intentionally preserved this pattern for the boundary.
2.8 src/rag_engine.py — INTERNAL_RETHROW sites (4, baseline)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 29 | _get_sentence_transformers (lazy import) |
except ModuleNotFoundError as e: (start of except) |
PATTERN_1 (composite) | The except body contains both a raise ImportError(LOCAL_RAG_INSTALL_HINT) from e (PATTERN_1: catch + convert + raise with better message) and a bare raise (PATTERN_2: re-raise original). The except itself is the boundary |
| 36 | _get_sentence_transformers (lazy import) |
raise e after sys.stderr.write(...) |
PATTERN_2 | Catch + log + re-raise: writes to stderr, then re-raises the original exception. The log is for observability; the re-raise preserves the traceback for the caller |
| 57 | BaseEmbeddingProvider.embed (abstract method) |
raise NotImplementedError() |
compliant | Abstract method pattern: the base class raises NotImplementedError to signal subclasses must implement. The audit script's _classify_raise heuristic misses this (the function is not __init__ and NotImplementedError doesn't match the AssertionError, ValueError, or assert check) |
| 75 | GeminiEmbeddingProvider.embed (validation) |
raise ImportError("google-genai is not installed") after if google_module is None |
compliant | Validation raise: if a required dependency is missing, raise with an actionable message. This is the "explicit precondition check" pattern (per styleguide's "Constructors that fail with programmer errors" guidance) |
Subtotals: 2 PATTERN_1/2 + 2 compliant + 0 migration-target.
Note (audit script bug, OUT OF SCOPE for this review pass): The audit script's visit_Try method has a bug — it iterates over node.handlers for adding findings but then visits children of only the LAST handler's body. This causes it to miss raise statements in the first except handler. The raise ImportError(LOCAL_RAG_INSTALL_HINT) from e at L31 (in the first except ModuleNotFoundError) is a legitimate PATTERN_1 site that the audit misses. Document for future audit script fix.
New heuristic candidates:
raise NotImplementedError()as the entire function body →INTERNAL_PROGRAMMER_RAISE(abstract method pattern; the current heuristic checks__init__but should also check the function is the entire body)if <var> is None: raise ImportError(...)or similar validation raise →INTERNAL_PROGRAMMER_RAISE(precondition check pattern)
2.9 src/app_controller.py — INTERNAL_RETHROW sites (3)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 1224 | AppController.__getattr__ (dunder guard) |
raise AttributeError(name) for names starting with _ or known dunder/sunder |
compliant | Standard Python __getattr__ pattern: must raise AttributeError for missing attributes so hasattr() returns False. This is a language requirement, not a code smell |
| 1250 | AppController.__getattr__ (default fallback) |
raise AttributeError(name) for any name not in _UI_FLAG_DEFAULTS |
compliant | Standard Python __getattr__ pattern (same as L1224). The _UI_FLAG_DEFAULTS set is a defensive guard for known UI flags; everything else gets the standard AttributeError |
| 2982 | load_context_preset (validation) |
raise KeyError(f"Context preset '{name}' not found.") after if name not in presets |
compliant | Validation raise: the user requested a preset that doesn't exist. The error message is actionable (includes the missing name). KeyError is in PROGRAMMER_ERROR_EXCEPTIONS but the function is not __init__; this is still a programmer-error pattern (the caller asked for a thing that doesn't exist) |
Subtotals: 3 compliant + 0 migration-target.
2.10 src/gui_2.py — INTERNAL_RETHROW sites (2)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 757 | App.__getattr__ (controller guard) |
if name == 'controller': raise AttributeError(name) |
compliant | Standard __getattr__ + delegation pattern: the App class delegates to the controller; the controller attribute is set externally, so __getattr__ raises AttributeError when it's not yet set (Python idiom for "not initialized yet") |
| 760 | App.__getattr__ (default fallback) |
raise AttributeError(name) (end of __getattr__) |
compliant | Standard __getattr__ pattern (same as app_controller L1224, L1250): raise AttributeError for any name that's not in the controller's interface |
Subtotals: 2 compliant + 0 migration-target.
2.11 src/api_hooks.py — INTERNAL_RETHROW sites (2)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 938 | WebSocketServer._run_loop (port-bind retry) |
except OSError as e: (start of except) |
PATTERN_2 | Composite site: the except body contains if attempt == max_retries - 1: logging.error(...); raise (log + re-raise after all retries fail). The except is the boundary for the retry-then-give-up pattern |
| 941 | WebSocketServer._run_loop (port-bind retry) |
raise (bare re-raise inside except) |
PATTERN_2 | Catch + log + re-raise: the bare raise is paired with logging.error(...) for the "all retries failed" path. The original OSError is preserved for the caller |
Subtotals: 2 PATTERN_2 + 0 migration-target (both are the same site; L938 is the except and L941 is the raise).
2.12 src/models.py — INTERNAL_RETHROW site (1)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 268 | models.__getattr__ (module-level PEP 562) |
raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
compliant | Standard module-level __getattr__ pattern (PEP 562): handles PROVIDERS and _PYDANTIC_CLASS_FACTORIES lookups, then raises AttributeError for everything else. Python idiom |
Subtotals: 1 compliant + 0 migration-target.
2.13 src/warmup.py — INTERNAL_RETHROW site (1)
| Line | Context | Snippet | Decision | Pattern / Rationale |
|---|---|---|---|---|
| 85 | WarmupManager.submit (double-submit guard) |
raise RuntimeError("WarmupManager.submit() called twice; call reset() first") |
compliant | Validation raise for double-submit guard: the user called submit twice without reset in between, which is a programming error (API misuse). The error message is actionable. RuntimeError is in PROGRAMMER_ERROR_EXCEPTIONS |
Subtotals: 1 compliant + 0 migration-target.
3. Post-Review Migration Scope
(filled in Task 5.1)
4. Audit Script Heuristic Updates
(filled in Task 4.1)
5. Verification
(filled in Phase 6)