From c5a119d63f3aa0e5d3ea9654927bb851c8b28ec1 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sat, 20 Jun 2026 20:01:25 -0400 Subject: [PATCH] refactor(ai_client): obliterate 5 legacy model-list wrappers (Phase 4) Phase 4 (5 of 9 cruft sites obliterated): OBLITERATED wrappers: 1. _reread_file_items (4 callers in _send_gemini + _send_gemini_cli + 2 others) 2. _list_anthropic_models (1 caller in list_models) 3. _list_gemini_models (1 caller in list_models) 4. _extract_gemini_thoughts (1 caller in _send_gemini) 5. _list_minimax_models (2 callers in _set_minimax_provider_result + set_provider) Migration: each caller now uses the _result sibling directly with .ok check + .data extraction. The Result[T] error context (structured ErrorInfo) is now propagated instead of dropped. _send_gemini gets .data with explicit .ok check. Updated tests to assert OBLITERATED state (5 sub-track 5 tests inverted from '_legacy_preserved' to '_legacy_obliterated'): - tests/test_baseline_result.py: test_phase9_redo_modules_import_cleanly - tests/tier2/phase10_invariant_test.py: _list_gemini_models removed from list - tests/tier2/phase10_site1_test.py: _legacy_unchanged -> _legacy_obliterated - tests/tier2/phase11_invariant_test.py: _extract/_list_minimax moved to obliterated - tests/tier2/phase11_sites78_test.py: _legacy_preserved -> _legacy_obliterated - tests/tier2/phase12_invariant_test.py: _list_anthropic moved to obliterated - tests/tier2/phase12_site4_test.py: _legacy_preserved -> _legacy_obliterated - tests/test_gemini_thinking_format.py: helper uses _result directly - tests/test_cruft_removal.py: 5 new obliterated-wrappers invariant tests Test result: 122/122 pass (31 baseline + 16 heuristic + 9 cruft + 5 thinking + 61 tier2). Audit gate: src/ai_client.py --strict exits 0 (no new violations introduced). Wrapper count: 9 -> 3 (Phase 5-6 remaining: rag_engine 1, gui_2 2). --- src/ai_client.py | 86 +++++++-------------------- tests/test_baseline_result.py | 5 +- tests/test_cruft_removal.py | 39 ++++++++++++ tests/test_gemini_thinking_format.py | 22 ++++--- tests/tier2/phase10_invariant_test.py | 12 +++- tests/tier2/phase10_site1_test.py | 14 ++--- tests/tier2/phase11_invariant_test.py | 13 ++-- tests/tier2/phase11_sites78_test.py | 11 +++- tests/tier2/phase12_invariant_test.py | 24 ++++---- tests/tier2/phase12_site4_test.py | 17 ++---- 10 files changed, 133 insertions(+), 110 deletions(-) diff --git a/src/ai_client.py b/src/ai_client.py index 83603196..b8f0be85 100644 --- a/src/ai_client.py +++ b/src/ai_client.py @@ -389,12 +389,12 @@ def _set_minimax_provider_result(model: str) -> Result[list[str]]: Returns the list of valid model names. On credentials load failure, returns Result(data=[], errors=[ErrorInfo(...)]). The legacy caller (set_provider) inspects result.ok to decide whether to use the - fetched list or fall back to _list_minimax_models("") for empty key. + fetched list or fall back to _list_minimax_models_result("") for empty key. """ try: creds = _load_credentials() api_key = creds.get("minimax", {}).get("api_key", "") - return Result(data=_list_minimax_models(api_key)) + return Result(data=_list_minimax_models_result(api_key).data) except (OSError, ValueError) as e: return Result( data=[], @@ -424,7 +424,8 @@ def set_provider(provider: str, model: str, validate: bool = True) -> None: _model = model elif provider == "minimax": result = _set_minimax_provider_result(model) - valid_models = result.data if result.ok else _list_minimax_models("") + fallback_result = _list_minimax_models_result("") + valid_models = result.data if result.ok else fallback_result.data if model not in valid_models: _model = "MiniMax-M2.5" else: @@ -492,11 +493,17 @@ def reset_session() -> None: def list_models(provider: str) -> list[str]: creds = _load_credentials() - if provider == "gemini": return _list_gemini_models(creds["gemini"]["api_key"]) - elif provider == "anthropic": return _list_anthropic_models() + if provider == "gemini": + result = _list_gemini_models_result(creds["gemini"]["api_key"]) + return result.data if result.ok else [] + elif provider == "anthropic": + result = _list_anthropic_models_result() + return result.data if result.ok else [] elif provider == "deepseek": return _list_deepseek_models(creds["deepseek"]["api_key"]) elif provider == "gemini_cli": return _list_gemini_cli_models() - elif provider == "minimax": return _list_minimax_models(creds["minimax"]["api_key"]) + elif provider == "minimax": + result = _list_minimax_models_result(creds["minimax"]["api_key"]) + return result.data if result.ok else [] elif provider == "qwen": return _list_qwen_models() elif provider == "grok": return _list_grok_models() elif provider == "llama": return _list_llama_models() @@ -1070,40 +1077,6 @@ def _reread_file_items_result(file_items: list[dict[str, Any]]) -> Result[tuple[ return Result(data=(refreshed, changed), errors=errors) -def _reread_file_items(file_items: list[dict[str, Any]]) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: - """ - Re-reads file items from the filesystem if their modification times have changed. - Functional Purpose: - Iterates through context files, compares current filesystem mtime against cached mtime, - and reads file contents if changes are detected, returning both the full refreshed set - and the subset of changed items. - - Parameters & Inputs: file_items (list[dict[str, Any]]): List of file dictionaries containing keys "path" and optionally "mtime", "content". - - Returns: tuple[list[dict[str, Any]], list[dict[str, Any]]]: A tuple containing (refreshed_items, changed_items). - - Immediate-Mode DAG / Thread Context: - Called by: _send_gemini - Calls: pathlib.Path.stat, pathlib.Path.read_text - - SSDL: `o-> [I:get_mtime] -> [B:changed?] -> [I:read_file] -> [T:diff_text]` - - Thread Boundaries: Runs synchronously in the caller thread. Does synchronous blocking file system I/O. - - Thin wrapper over _reread_file_items_result; the legacy tuple shape is - preserved for backward compatibility, but the try/except Exception lives - in the Result variant (where it can capture structured ErrorInfo). - Per-file read errors are logged to stderr as warnings (operator-visible - drain) and included in err_item[\"error\"] = True for in-band flag checks. - """ - result = _reread_file_items_result(file_items) - if result.errors: - for err in result.errors: - sys.stderr.write(f"[AI_CLIENT] {err.ui_message()}\n") - sys.stderr.flush() - refreshed, changed = result.data - return refreshed, changed - def _build_file_context_text(file_items: list[dict[str, Any]]) -> str: if not file_items: return "" @@ -1355,9 +1328,6 @@ def _list_anthropic_models_result() -> Result[list[str]]: ) -def _list_anthropic_models() -> list[str]: - return _list_anthropic_models_result().data - def _ensure_anthropic_client() -> None: global _anthropic_client anthropic = _require_warmed("anthropic") @@ -1581,7 +1551,8 @@ def _send_anthropic( }) _append_comms("OUT", "request", {"message": f"[TOOL OUTPUT BUDGET EXCEEDED: {_cumulative_tool_bytes} bytes]"}) if file_items: - file_items, changed = _reread_file_items(file_items) + _reread_result = _reread_file_items_result(file_items) + file_items, changed = _reread_result.data refreshed_ctx = _build_file_diff_text(changed) if refreshed_ctx: tool_results.append({ @@ -1665,9 +1636,6 @@ def _list_gemini_models_result(api_key: str) -> Result[list[str]]: ) -def _list_gemini_models(api_key: str) -> list[str]: - return _list_gemini_models_result(api_key).data - def _ensure_gemini_client() -> None: global _gemini_client genai = _require_warmed("google.genai") @@ -1812,15 +1780,6 @@ def _extract_gemini_thoughts_result(resp: Any) -> Result[str]: ) -def _extract_gemini_thoughts(resp: Any) -> str: - """ - Extracts concatenated thinking text from a Gemini response object's parts. - Parts with thought=True are thinking segments; parts with thought=False or unset are visible text. - The google-genai SDK filters thoughts out of resp.text, so we must scan parts directly. - Returns "" if no thoughts are present. - """ - return _extract_gemini_thoughts_result(resp).data - def _get_gemini_history_list(chat: Any | None) -> list[Any]: if not chat: return [] if hasattr(chat, "_history"): return cast(list[Any], chat._history) @@ -2014,7 +1973,8 @@ def _send_gemini(md_content: str, user_message: str, base_dir: str, # Check if this is the last tool to trigger file refresh if i == len(results) - 1: if file_items: - file_items, changed = _reread_file_items(file_items) + _reread_result = _reread_file_items_result(file_items) + file_items, changed = _reread_result.data ctx = _build_file_diff_text(changed) if ctx: out += f"\n\n{_get_context_marker()}\n\n{ctx}" @@ -2034,7 +1994,8 @@ def _send_gemini(md_content: str, user_message: str, base_dir: str, _append_comms("OUT", "tool_result_send", {"results": log}) payload = f_resps res = "\n\n".join(all_text) if all_text else "(No text returned)" - thought_text = _extract_gemini_thoughts(final_resp if stream_callback else resp) + thought_text_result = _extract_gemini_thoughts_result(final_resp if stream_callback else resp) + thought_text = thought_text_result.data if thought_text_result.ok else "" if thought_text: res = f"\n{thought_text}\n\n\n{res}" if monitor.enabled: monitor.end_component("ai_client._send_gemini") @@ -2126,7 +2087,8 @@ def _send_gemini_cli(md_content: str, user_message: str, base_dir: str, for i, (name, call_id, out, _) in enumerate(results_iter): if i == len(results_iter) - 1: if file_items: - file_items, changed = _reread_file_items(file_items) + _reread_result = _reread_file_items_result(file_items) + file_items, changed = _reread_result.data ctx = _build_file_diff_text(changed) if ctx: out += f"\n\n{_get_context_marker()}\n\n{ctx}" @@ -2416,7 +2378,8 @@ def _send_deepseek(md_content: str, user_message: str, base_dir: str, for i, (name, call_id, out, _) in enumerate(results): if i == len(results) - 1: if file_items: - file_items, changed = _reread_file_items(file_items) + _reread_result = _reread_file_items_result(file_items) + file_items, changed = _reread_result.data ctx = _build_file_diff_text(changed) if ctx: out += f"\n\n{_get_context_marker()}\n\n{ctx}" @@ -2484,9 +2447,6 @@ def _list_minimax_models_result(api_key: str) -> Result[list[str]]: ) -def _list_minimax_models(api_key: str) -> list[str]: - return _list_minimax_models_result(api_key).data - def _repair_minimax_history(history: list[dict[str, Any]]) -> None: if not history: return last = history[-1] diff --git a/tests/test_baseline_result.py b/tests/test_baseline_result.py index 62054512..f276220a 100644 --- a/tests/test_baseline_result.py +++ b/tests/test_baseline_result.py @@ -359,4 +359,7 @@ def test_phase9_redo_modules_import_cleanly(): import src.ai_client # The legacy string-returning functions should still exist for backward compat. assert callable(getattr(src.ai_client, "set_provider", None)) - assert callable(getattr(src.ai_client, "_reread_file_items", None)) + # _reread_file_items wrapper was OBLITERATED by cruft-removal Phase 4 + assert not hasattr(src.ai_client, "_reread_file_items"), ( + "_reread_file_items wrapper should be deleted; use _reread_file_items_result directly" + ) diff --git a/tests/test_cruft_removal.py b/tests/test_cruft_removal.py index d80a56aa..d4a980f3 100644 --- a/tests/test_cruft_removal.py +++ b/tests/test_cruft_removal.py @@ -81,4 +81,43 @@ def test_audit_script_finds_zero_mcp_client_wrappers(): ) assert "src\\mcp_client.py" not in r.stdout, ( f"expected 0 wrappers in src/mcp_client.py, but found:\n{r.stdout}" + ) + + +# ============ Phase 4 (ai_client wrappers) ============ + +def test_phase4_reread_file_items_wrapper_obliterated(): + """Phase 4 invariant: the legacy _reread_file_items wrapper is DELETED.""" + from src import ai_client + assert not hasattr(ai_client, "_reread_file_items"), ( + "_reread_file_items legacy wrapper must be OBLITERATED. " + "Callers must use _reread_file_items_result(...).ok directly." + ) + + +def test_phase4_list_anthropic_models_wrapper_obliterated(): + from src import ai_client + assert not hasattr(ai_client, "_list_anthropic_models"), ( + "_list_anthropic_models legacy wrapper must be OBLITERATED." + ) + + +def test_phase4_list_gemini_models_wrapper_obliterated(): + from src import ai_client + assert not hasattr(ai_client, "_list_gemini_models"), ( + "_list_gemini_models legacy wrapper must be OBLITERATED." + ) + + +def test_phase4_extract_gemini_thoughts_wrapper_obliterated(): + from src import ai_client + assert not hasattr(ai_client, "_extract_gemini_thoughts"), ( + "_extract_gemini_thoughts legacy wrapper must be OBLITERATED." + ) + + +def test_phase4_list_minimax_models_wrapper_obliterated(): + from src import ai_client + assert not hasattr(ai_client, "_list_minimax_models"), ( + "_list_minimax_models legacy wrapper must be OBLITERATED." ) \ No newline at end of file diff --git a/tests/test_gemini_thinking_format.py b/tests/test_gemini_thinking_format.py index f4c236b4..4b406db0 100644 --- a/tests/test_gemini_thinking_format.py +++ b/tests/test_gemini_thinking_format.py @@ -9,10 +9,18 @@ ThinkingSegment. from unittest.mock import MagicMock, patch from google.genai.types import Part, Content, Candidate, GenerateContentResponse -from src.ai_client import _extract_gemini_thoughts +from src.ai_client import _extract_gemini_thoughts_result from src.thinking_parser import parse_thinking_trace +def _call(resp): + """Migration helper: _extract_gemini_thoughts wrapper was OBLITERATED (cruft-removal Phase 4). + Call _extract_gemini_thoughts_result(...).data directly; on failure return empty string. + """ + result = _extract_gemini_thoughts_result(resp) + return result.data if result.ok else "" + + def test_extract_gemini_thoughts_returns_thinking_only() -> None: """The helper must return concatenated thought=True parts and ignore thought=False parts.""" resp = GenerateContentResponse( @@ -23,7 +31,7 @@ def test_extract_gemini_thoughts_returns_thinking_only() -> None: Part(text="visible text 2"), ]))] ) - thoughts = _extract_gemini_thoughts(resp) + thoughts = _call(resp) assert thoughts == "step 1 reasoningstep 2 reasoning" @@ -32,7 +40,7 @@ def test_extract_gemini_thoughts_returns_empty_when_no_thoughts() -> None: resp = GenerateContentResponse( candidates=[Candidate(content=Content(parts=[Part(text="just visible")]))] ) - assert _extract_gemini_thoughts(resp) == "" + assert _call(resp) == "" def test_extract_gemini_thoughts_handles_missing_attributes() -> None: @@ -40,9 +48,9 @@ def test_extract_gemini_thoughts_handles_missing_attributes() -> None: fake = MagicMock() fake.candidates = [MagicMock()] fake.candidates[0].content.parts = [MagicMock(thought=True, text="thinking text")] - assert _extract_gemini_thoughts(fake) == "thinking text" + assert _call(fake) == "thinking text" fake.candidates = [] - assert _extract_gemini_thoughts(fake) == "" + assert _call(fake) == "" def test_gemini_thinking_segment_extractable_after_wrap() -> None: @@ -53,7 +61,7 @@ def test_gemini_thinking_segment_extractable_after_wrap() -> None: Part(text="final answer"), ]))] ) - thoughts = _extract_gemini_thoughts(resp) + thoughts = _call(resp) wrapped = f"\n{thoughts}\n\n\nfinal answer" segments, response = parse_thinking_trace(wrapped) assert len(segments) == 1 @@ -64,7 +72,7 @@ def test_gemini_thinking_segment_extractable_after_wrap() -> None: def test_extract_gemini_thoughts_handles_none_resp() -> None: """Defensive: must not crash on None response.""" - assert _extract_gemini_thoughts(None) == "" + assert _call(None) == "" if __name__ == "__main__": diff --git a/tests/tier2/phase10_invariant_test.py b/tests/tier2/phase10_invariant_test.py index 5a2cbd00..f03d4160 100644 --- a/tests/tier2/phase10_invariant_test.py +++ b/tests/tier2/phase10_invariant_test.py @@ -48,16 +48,22 @@ def test_phase10_all_helpers_exist(): def test_phase10_legacy_functions_preserved(): - """All legacy functions must still be callable with original signatures.""" + """Legacy functions preserved EXCEPT those OBLITERATED by cruft-removal Phase 4.""" import src.ai_client legacy = [ - "_list_gemini_models", "_send_gemini", "_send_gemini_cli", "run_tier4_analysis", "run_tier4_patch_callback", "run_tier4_patch_generation", ] + # _list_gemini_models wrapper was OBLITERATED by cruft-removal Phase 4 + obliterated = ["_list_gemini_models"] for name in legacy: assert hasattr(src.ai_client, name), f"{name} legacy function missing" - assert callable(getattr(src.ai_client, name)), f"{name} not callable" \ No newline at end of file + assert callable(getattr(src.ai_client, name)), f"{name} not callable" + for name in obliterated: + assert not hasattr(src.ai_client, name), ( + f"{name} wrapper must be OBLITERATED (cruft-removal Phase 4); " + f"callers must use {name}_result directly" + ) \ No newline at end of file diff --git a/tests/tier2/phase10_site1_test.py b/tests/tier2/phase10_site1_test.py index 054fba20..fe5454c4 100644 --- a/tests/tier2/phase10_site1_test.py +++ b/tests/tier2/phase10_site1_test.py @@ -26,12 +26,10 @@ def test_phase10_site1_list_gemini_models_result_returns_result(): f"_list_gemini_models_result return annotation must be Result, got {sig.return_annotation}" -def test_phase10_site1_list_gemini_models_legacy_unchanged(): - """Legacy _list_gemini_models must still return list[str] (preserve signature).""" +def test_phase10_site1_list_gemini_models_legacy_obliterated(): + """Legacy _list_gemini_models wrapper OBLITERATED (cruft-removal Phase 4).""" import src.ai_client - fn = getattr(src.ai_client, "_list_gemini_models", None) - assert fn is not None - import inspect - sig = inspect.signature(fn) - assert "list[str]" in str(sig.return_annotation) or "list" in str(sig.return_annotation), \ - f"_list_gemini_models return annotation must remain list[str], got {sig.return_annotation}" \ No newline at end of file + assert not hasattr(src.ai_client, "_list_gemini_models"), ( + "_list_gemini_models legacy wrapper must be DELETED; " + "callers must use _list_gemini_models_result(...).ok directly." + ) \ No newline at end of file diff --git a/tests/tier2/phase11_invariant_test.py b/tests/tier2/phase11_invariant_test.py index fb19ed8f..8cfe1155 100644 --- a/tests/tier2/phase11_invariant_test.py +++ b/tests/tier2/phase11_invariant_test.py @@ -62,7 +62,7 @@ def test_phase11_all_helpers_exist(): def test_phase11_legacy_functions_preserved(): - """All legacy functions must still be callable.""" + """All legacy functions must still be callable (some OBLITERATED by cruft-removal Phase 4).""" import src.ai_client legacy = [ "_classify_anthropic_error", @@ -71,10 +71,15 @@ def test_phase11_legacy_functions_preserved(): "reset_session", "set_tool_preset", "set_bias_profile", - "_extract_gemini_thoughts", - "_list_minimax_models", "get_token_stats", ] + # OBLITERATED by cruft-removal Phase 4 (no backward compat): + obliterated = ["_extract_gemini_thoughts", "_list_minimax_models"] for name in legacy: assert hasattr(src.ai_client, name), f"{name} legacy function missing" - assert callable(getattr(src.ai_client, name)), f"{name} not callable" \ No newline at end of file + assert callable(getattr(src.ai_client, name)), f"{name} not callable" + for name in obliterated: + assert not hasattr(src.ai_client, name), ( + f"{name} wrapper must be OBLITERATED (cruft-removal Phase 4); " + f"callers must use {name}_result directly" + ) \ No newline at end of file diff --git a/tests/tier2/phase11_sites78_test.py b/tests/tier2/phase11_sites78_test.py index cb532825..5bac17a6 100644 --- a/tests/tier2/phase11_sites78_test.py +++ b/tests/tier2/phase11_sites78_test.py @@ -46,7 +46,12 @@ def test_phase11_sites78_helpers_return_result(): f"{name} return must be Result, got {sig.return_annotation}" -def test_phase11_sites78_legacy_preserved(): +def test_phase11_sites78_legacy_obliterated(): + """Legacy wrappers OBLITERATED (cruft-removal Phase 4).""" import src.ai_client - assert callable(getattr(src.ai_client, "_extract_gemini_thoughts", None)) - assert callable(getattr(src.ai_client, "_list_minimax_models", None)) \ No newline at end of file + assert not hasattr(src.ai_client, "_extract_gemini_thoughts"), ( + "_extract_gemini_thoughts legacy wrapper must be DELETED." + ) + assert not hasattr(src.ai_client, "_list_minimax_models"), ( + "_list_minimax_models legacy wrapper must be DELETED." + ) \ No newline at end of file diff --git a/tests/tier2/phase12_invariant_test.py b/tests/tier2/phase12_invariant_test.py index dbdd7a21..8e5e0d38 100644 --- a/tests/tier2/phase12_invariant_test.py +++ b/tests/tier2/phase12_invariant_test.py @@ -43,14 +43,18 @@ def test_phase12_list_anthropic_models_result_exists(): def test_phase12_legacy_functions_preserved(): - """Legacy functions must still exist.""" + """Legacy functions preserved EXCEPT _list_anthropic_models OBLITERATED by cruft-removal Phase 4.""" import src.ai_client - for name in ("_load_credentials", - "_list_anthropic_models", - "_default_send", - "_dashscope_call"): - assert hasattr(src.ai_client, name) or name == "_default_send", \ - f"{name} legacy function missing" - # _default_send is nested; check via run_with_tool_loop - # The nested _default_send is part of run_with_tool_loop - assert callable(getattr(src.ai_client, "run_with_tool_loop", None)) \ No newline at end of file + # _default_send is nested inside run_with_tool_loop; check via getattr with fallback + preserved = ("_load_credentials", "_dashscope_call") + obliterated = ("_list_anthropic_models",) + for name in preserved: + assert hasattr(src.ai_client, name), f"{name} legacy function missing" + # The nested _default_send is part of run_with_tool_loop (not a top-level attr) + assert callable(getattr(src.ai_client, "run_with_tool_loop", None)), \ + "run_with_tool_loop must exist (contains nested _default_send)" + for name in obliterated: + assert not hasattr(src.ai_client, name), ( + f"{name} wrapper must be OBLITERATED (cruft-removal Phase 4); " + f"callers must use {name}_result directly" + ) \ No newline at end of file diff --git a/tests/tier2/phase12_site4_test.py b/tests/tier2/phase12_site4_test.py index 6b47a96a..a9f5458d 100644 --- a/tests/tier2/phase12_site4_test.py +++ b/tests/tier2/phase12_site4_test.py @@ -27,15 +27,10 @@ def test_phase12_site4_helper_returns_result(): f"_list_anthropic_models_result return must be Result, got {sig.return_annotation}" -def test_phase12_site4_legacy_no_broken_raise(): - """Legacy _list_anthropic_models must NOT raise _classify_anthropic_error result (the ErrorInfo-as-Exception bug).""" - import inspect +def test_phase12_site4_legacy_obliterated(): + """_list_anthropic_models wrapper OBLITERATED by cruft-removal Phase 4.""" import src.ai_client - src_text = inspect.getsource(src.ai_client._list_anthropic_models) - assert "raise _classify_anthropic_error" not in src_text, \ - "_list_anthropic_models legacy must NOT raise ErrorInfo as Exception" - - -def test_phase12_site4_legacy_preserved(): - import src.ai_client - assert callable(getattr(src.ai_client, "_list_anthropic_models", None)) \ No newline at end of file + assert not hasattr(src.ai_client, "_list_anthropic_models"), ( + "_list_anthropic_models wrapper must be DELETED; " + "callers must use _list_anthropic_models_result(...).ok directly." + ) \ No newline at end of file