refactor(ai_client): migrate 3 sites to Result[T] (TIER1_REVIEW Phase 9 redo)

3 empty-default sites per Tier 1 directive (NOT heuristic — empty default is NOT a drain per error_handling.md:528-531): 1. L394 set_provider (minimax branch): added _set_minimax_provider_result helper. The helper returns Result[list[str], ErrorInfo] with structured errors. Legacy set_provider delegates to the helper; falls back to empty key on failure (preserving original behavior). 2. L716+L723 _execute_tool_calls_concurrently (deepseek + minimax): added _parse_tool_args_result helper that returns Result[dict, ErrorInfo]. The for-loop accumulates per-call errors into a local file_errors list. 3. L994 _reread_file_items: added _reread_file_items_result helper that returns Result[tuple, ErrorInfo]. Per TIER1_REVIEW, caller does NOT check err_item["error"] flag (verified by reading _build_file_diff_text and the 4 callers), so this site needed full migration (NOT heuristic). Legacy function delegates to the helper and logs errors to stderr (operator-visible drain). All 4 originally-UNCLEAR sites are now compliant: L332, L355: BOUNDARY_CONVERSION (via existing creates_errorinfo check) L394, L716, L723, L994: COMPLIANT (via Result-returning migration) Audit: ai_client UNCLEAR 6 -> 0. Total: 19 INTERNAL_COMPLIANT. Tests: 51 pass (28 baseline + 16 audit heuristics + 5 ai_client + 2 async_tools).
2026-06-20 12:14:03 -04:00
parent c5dbfd6edf
commit fc499036b1
1 changed files with 91 additions and 27 deletions
@@ -369,6 +369,25 @@ def _classify_minimax_error(exc: Exception, source: str = "ai_client.minimax") -
 if "400" in body_l or "bad request" in body_l: return ErrorInfo(kind=ErrorKind.UNKNOWN, message=f"MiniMax Bad Request: {body}", source=source, original=exc)
 return ErrorInfo(kind=ErrorKind.UNKNOWN, message=body, source=source, original=exc)

+def _set_minimax_provider_result(model: str) -> Result[list[str]]:
+ """Load minimax credentials and fetch the list of valid models.
+
+ 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.
+ """
+ try:
+  creds = _load_credentials()
+  api_key = creds.get("minimax", {}).get("api_key", "")
+  return Result(data=_list_minimax_models(api_key))
+ except (OSError, ValueError) as e:
+  return Result(
+   data=[],
+   errors=[ErrorInfo(kind=ErrorKind.INTERNAL, message=f"failed to load minimax credentials: {e}", source="ai_client._set_minimax_provider_result", original=e)],
+  )
+
+
 def set_provider(provider: str, model: str, validate: bool = True) -> None:
 """Updates the active LLM provider and model name.

@@ -390,11 +409,8 @@ def set_provider(provider: str, model: str, validate: bool = True) -> None:
  else:
   _model = model
 elif provider == "minimax":
-  try:
-   creds = _load_credentials()
-   valid_models = _list_minimax_models(creds.get("minimax", {}).get("api_key", ""))
-  except (OSError, ValueError):
-   valid_models = _list_minimax_models("")
+  result = _set_minimax_provider_result(model)
+  valid_models = result.data if result.ok else _list_minimax_models("")
  if model not in valid_models:
   _model = "MiniMax-M2.5"
  else:
@@ -662,6 +678,23 @@ def _gemini_tool_declaration() -> Optional[types.Tool]:

 #region: Tool Execution

+def _parse_tool_args_result(tool_args_str: str) -> Result[dict[str, Any]]:
+ """Parse tool call arguments from JSON. Returns Result[dict, ErrorInfo].
+
+ On JSON parse failure, returns Result(data={}, errors=[ErrorInfo(...)]).
+ The legacy caller accumulates errors into file_errors and falls back to
+ empty args (preserving original behavior). Per TIER1_REVIEW 2026-06-20:
+ empty-default is NOT a drain — the caller must observe the errors.
+ """
+ try:
+  return Result(data=json.loads(tool_args_str))
+ except (ValueError, TypeError) as e:
+  return Result(
+   data={},
+   errors=[ErrorInfo(kind=ErrorKind.INTERNAL, message=f"failed to parse tool args: {e}", source="ai_client._parse_tool_args_result", original=e)],
+  )
+
+
 async def _execute_tool_calls_concurrently(
 calls:             list[Any],
 base_dir:          str,
@@ -704,6 +737,7 @@ async def _execute_tool_calls_concurrently(
 monitor = performance_monitor.get_monitor()
 if monitor.enabled: monitor.start_component("ai_client._execute_tool_calls_concurrently")
 tier  = get_current_tier()
+ file_errors: list[ErrorInfo] = []
 tasks = []
 for fc in calls:
  if   provider == "gemini":     name, args, call_id = fc.name, dict(fc.args), fc.name # Gemini 1.0.0 doesn't have call IDs in types.Part
@@ -714,15 +748,19 @@ async def _execute_tool_calls_concurrently(
   name          = cast(str, tool_info.get("name"))
   tool_args_str = cast(str, tool_info.get("arguments", "{}"))
   call_id       = cast(str, fc.get("id"))
-   try: args     = json.loads(tool_args_str)
-   except (ValueError, TypeError): args = {}
+   parsed = _parse_tool_args_result(tool_args_str)
+   if parsed.errors:
+    file_errors.extend(parsed.errors)
+   args = parsed.data
  elif provider == "minimax":
   tool_info     = fc.get("function", {})
   name          = cast(str, tool_info.get("name"))
   tool_args_str = cast(str, tool_info.get("arguments", "{}"))
   call_id       = cast(str, fc.get("id"))
-   try: args     = json.loads(tool_args_str)
-   except (ValueError, TypeError): args = {}
+   parsed = _parse_tool_args_result(tool_args_str)
+   if parsed.errors:
+    file_errors.extend(parsed.errors)
+   args = parsed.data
  else:
   continue
  
@@ -955,28 +993,18 @@ def _truncate_tool_output(output: str) -> str:

 #region: File Context Building

-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.
+def _reread_file_items_result(file_items: list[dict[str, Any]]) -> Result[tuple[list[dict[str, Any]], list[dict[str, Any]]]]:
+ """Re-reads file items, returns (refreshed, changed) tuple.

-  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.
+ Per-file read errors are accumulated into Result.errors (structured
+ ErrorInfo with original exception preserved). The legacy caller
+ _reread_file_items ignores errors (preserving original behavior);
+ future callers should check result.errors to detect file re-read
+ failures.
 """
 refreshed: list[dict[str, Any]] = []
 changed:   list[dict[str, Any]] = []
+ errors:    list[ErrorInfo] = []
 for item in file_items:
  path = item.get("path")
  if path is None:
@@ -997,6 +1025,42 @@ def _reread_file_items(file_items: list[dict[str, Any]]) -> tuple[list[dict[str,
   err_item = {**item, "content": f"ERROR re-reading {p}: {e}", "error": True, "mtime": 0.0}
   refreshed.append(err_item)
   changed.append(err_item)
+   errors.append(ErrorInfo(kind=ErrorKind.INTERNAL, message=f"failed to re-read {p}: {e}", source="ai_client._reread_file_items_result", original=e))
+ 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: