From 8c4791d03f43006e779f097b6ceb2cdf9567f524 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sat, 6 Jun 2026 18:30:44 -0400 Subject: [PATCH] fix(ai_client,module_loader): pre-existing bugs surfaced by Phase 3 refactor Three test failures identified by the batched test suite, all rooted in the Phase 3 lazy-import refactor of src/ai_client.py. FIX 1: UnboundLocalError in _ensure_gemini_client - _ensure_gemini_client had a latent bug: creds was assigned inside `if _gemini_client is None:` but used on the next line. When the client was already cached, the assignment was skipped and the next line raised UnboundLocalError. Moved the Client() construction inside the if block to match creds' scope. - This affected test_ai_cache_tracking.py and (downstream) test_gui_updates.py::test_telemetry_data_updates_correctly. FIX 2: Phase 3 removed top-level `import requests` from ai_client.py. - test_discussion_compression.py::test_discussion_compression_deepseek did `patch("src.ai_client.requests.post", ...)` which no longer works. - Updated the test to mock _require_warmed to return a fake requests module with `.post()`, matching the new lazy-import pattern. FIX 3: _require_warmed could not import dotted names like `google.genai.types` - The google-genai library has a self-referential __init__.py that does `from .client import Client` which transitively does `from .types import HttpOptions`. Importing `google.genai.types` FIRST (before the parent package is fully loaded) hit a "partially initialized module" circular import. - Enhanced _require_warmed to pre-import parent packages for dotted names: walks `name.split(".")` and imports each parent (if not in sys.modules) before the leaf import. O(n) extra imports per call on first use; subsequent calls are O(1) sys.modules hit. TESTS: - test_ai_cache_tracking.py: 2/2 PASS - test_discussion_compression.py: 4/4 PASS - 29/29 PASS across the sampled test files that were failing (test_subagent_summarization, test_tool_access_exclusion, test_tier4_interceptor, test_gui2_mcp, test_gui_updates, test_headless_service) ARCHITECTURAL NOTE: The _require_warmed enhancement is a small but important robustness fix. The google-genai library's __init__.py chain is a known source of fragility; the parent- pre-import pattern is the recommended workaround. --- src/ai_client.py | 2 +- src/module_loader.py | 15 +++++++++++++++ tests/test_discussion_compression.py | 9 ++++++++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/ai_client.py b/src/ai_client.py index 47c33ded..0e3e7a7a 100644 --- a/src/ai_client.py +++ b/src/ai_client.py @@ -1395,7 +1395,7 @@ def _ensure_gemini_client() -> None: genai = _require_warmed("google.genai") if _gemini_client is None: creds = _load_credentials() - _gemini_client = genai.Client(api_key=creds["gemini"]["api_key"]) + _gemini_client = genai.Client(api_key=creds["gemini"]["api_key"]) def _get_gemini_history_list(chat: Any | None) -> list[Any]: if not chat: return [] diff --git a/src/module_loader.py b/src/module_loader.py index f62f080f..5094111e 100644 --- a/src/module_loader.py +++ b/src/module_loader.py @@ -46,8 +46,23 @@ def _require_warmed(name: str) -> Any: In production: this is an O(1) sys.modules lookup. The 1+ second import cost is paid during startup on a bg thread, NOT on the first user-triggered AI call. + + For dotted names (e.g. 'google.genai.types'), the parent packages + are pre-imported first. This avoids circular-import errors in libraries + like google-genai where importing a sub-module triggers a chain that + re-enters the same sub-module (e.g. google.genai._api_client does + 'from .types import HttpOptions' during its own initialization). + Without parent pre-import, that relative import would see a + 'partially initialized' google.genai.types in sys.modules and raise + ImportError. """ mod = sys.modules.get(name) if mod is not None: return mod + if "." in name: + parts = name.split(".") + for i in range(1, len(parts)): + parent = ".".join(parts[:i]) + if parent not in sys.modules: + importlib.import_module(parent) return importlib.import_module(name) diff --git a/tests/test_discussion_compression.py b/tests/test_discussion_compression.py index 027361c4..7f727f16 100644 --- a/tests/test_discussion_compression.py +++ b/tests/test_discussion_compression.py @@ -51,7 +51,14 @@ def test_discussion_compression_deepseek(): mock_response = MagicMock() mock_response.json.return_value = {"choices": [{"message": {"content": "DeepSeek summary."}}]} - with patch("src.ai_client.requests.post", return_value=mock_response), \ + # Phase 3 (startup_speedup_20260606) removed the top-level 'import requests' + # from src/ai_client.py. The module is now resolved via _require_warmed() + # at call time. We mock _require_warmed to return a fake requests module + # that has the .post() method. + mock_requests_mod = MagicMock() + mock_requests_mod.post.return_value = mock_response + + with patch("src.ai_client._require_warmed", return_value=mock_requests_mod), \ patch("src.ai_client._load_credentials", return_value={"deepseek": {"api_key": "test"}}): result = ai_client.run_discussion_compression("DeepSeek history")