Private
Public Access
0
0

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).
This commit is contained in:
2026-06-20 20:01:25 -04:00
parent da7ac0ddb3
commit c5a119d63f
10 changed files with 133 additions and 110 deletions
+23 -63
View File
@@ -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"<thinking>\n{thought_text}\n</thinking>\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]
+4 -1
View File
@@ -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"
)
+39
View File
@@ -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."
)
+15 -7
View File
@@ -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"<thinking>\n{thoughts}\n</thinking>\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__":
+9 -3
View File
@@ -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"
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"
)
+6 -8
View File
@@ -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}"
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."
)
+9 -4
View File
@@ -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"
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"
)
+8 -3
View File
@@ -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))
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."
)
+14 -10
View File
@@ -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))
# _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"
)
+6 -11
View File
@@ -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))
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."
)