Phase 2 deferred t2_6: update src/ai_client.py _send_grok + _send_minimax +
_send_llama + _send_gemini_cli (4 functions) to use the new
dataclass API after NormalizedResponse was refactored to
(text, tool_calls: tuple[ToolCall, ...], usage: UsageStats, raw_response).
These 4 callers were left with the old keyword args
(usage_input_tokens, usage_output_tokens, ...) which broke at
runtime: ai_client.send() raised
TypeError: NormalizedResponse.__init__() got an unexpected keyword
argument 'usage_input_tokens'.
FIXES:
- src/ai_client.py L2054: gemini_cli 'adapter unavailable' branch
- src/ai_client.py L2088: gemini_cli normal response branch
- Added: from src.openai_schemas import UsageStats (module level)
- Added backward-compat in src/openai_compatible.py:
messages_dicts = [m.to_dict() if hasattr(m, 'to_dict') else m for m in request.messages]
(accepts both ChatMessage dataclass and dict for backward compat
with existing tests that pass raw dicts)
TEST FIXES:
- tests/test_ai_client_tool_loop.py: _make_normalized_response helper
uses UsageStats instead of usage_*_tokens kwargs
- tests/test_ai_client_tool_loop_builder.py: same
- tests/test_ai_client_tool_loop_send_func.py: same
- tests/test_openai_compatible.py: NormalizedResponse(text=..., usage=UsageStats(...))
+ tool_calls[0].function.name (attribute access) instead of ['function']['name']
- tests/test_auto_whitelist.py: use update_session_metadata() instead of
dict subscript assignment (Session dataclass doesn't support item assignment)
VERIFIED:
uv run pytest tests/test_ai_client_*.py tests/test_openai_*.py \
tests/test_auto_whitelist.py --timeout=30
56 passed in 4.49s (19 previously failing tests now pass)
uv run python scripts/audit_weak_types.py --strict
STRICT OK: 115 weak sites <= baseline 115
uv run python scripts/audit_dataclass_coverage.py --strict
STRICT OK: 200 weak sites <= baseline 207
This commit closes the t2_6 deferred task. The 41-site Phase 3 call-site
migration remains deferred (separate provider_state_migration track).
Phase 1 of any_type_componentization_20260621. Migrates ai_client.py:
- Line 560: new_tools = {name: False for name in mcp_client.TOOL_NAMES}
-> mcp_tool_specs.tool_names()
- Line 582: _agent_tools = {name: True for name in mcp_client.TOOL_NAMES}
-> mcp_tool_specs.tool_names()
- Line 1012: is_native = name in mcp_client.TOOL_NAMES
-> name in mcp_tool_specs.tool_names()
Plus adds: from src import mcp_tool_specs
Verified:
uv run pytest tests/test_mcp_tool_specs.py tests/test_mcp_client_beads.py tests/test_mcp_client_paths.py tests/test_audit_dataclass_coverage.py tests/test_type_aliases.py
39 passed in 11.79s
No regressions. The mcp_client.TOOL_NAMES re-export is preserved for
backward compatibility with any external test/code that imports it.
Bug: Phase 11 sites 5+6 migration extracted _set_tool_preset_result and
_set_bias_profile_result helpers. The _set_tool_preset_result helper
modifies _active_tool_preset, _tool_approval_modes, _agent_tools without
declaring them as global, which causes the assignments to create LOCAL
variables instead of modifying the module-level globals.
This regression broke tests/test_bias_integration.py::test_set_tool_preset_with_objects:
preset = ToolPreset(name='ObjTest', categories={'General': [Tool(name='read_file', approval='auto')]})
with patch('src.tool_presets.ToolPresetManager.load_all', return_value={'ObjTest': preset}):
ai_client.set_tool_preset('ObjTest')
assert ai_client._agent_tools['read_file'] is True
# Fails: KeyError 'read_file' (the helper created a local _agent_tools,
# not modifying the module global; set_tool_preset legacy then ran
# cache-invalidation but never assigned _agent_tools to the test's view)
Fix: Add 'global _active_tool_preset, _tool_approval_modes, _agent_tools'
declaration to _set_tool_preset_result. The original set_tool_preset had
this declaration at the top; the helper extraction lost it.
Audit: no audit change (the helper still classifies as BOUNDARY_CONVERSION
via Heuristic A 'returns Result' pattern).
Per styleguide §7.6 Pattern 1: 'catch + convert + raise as different type'
requires 'raise X from e' to preserve the original exception in the
traceback.
Sites updated:
Site 1 (L277 _load_credentials):
except FileNotFoundError as e:
raise FileNotFoundError(f'...') from e
Sites 2+3 (L878+L879 _default_send, nested in run_with_tool_loop):
if not res.ok:
raise res.errors[0].original from None
raise RuntimeError(...) from None
The exceptions come from a Result, not a local except; 'from None'
suppresses the implicit context.
Site 5 (L2061 _send inside _send_gemini_cli):
raise cast(Exception, send_result.errors[0].original) from None
Site 6 (L2742 _dashscope_call):
raise classify_dashscope_error(_dashscope_exception_from_response(resp)) from None
KNOWN LIMITATION: the audit script does not have a heuristic for
'raise X from e' / 'from None' (Pattern 1). The sites remain
INTERNAL_RETHROW in the audit. INTERNAL_RETHROW is 'suspicious but
not violation' (strict mode accepts). Adding a heuristic requires
Tier 1 approval per the conventions.
Audit: ai_client RETHROW 6 -> 5 (site 4 migrated separately; these
4 sites stay as INTERNAL_RETHROW by audit classification but follow
Pattern 1 by styleguide).
Both classify functions had:
try:
sdk = _require_warmed('xxx')
if isinstance(exc, sdk.SomeException): return ErrorInfo(...)
...
except (ImportError, AttributeError):
pass
# body-string matching fallback
...
Body: bare 'except: pass' = SS violation (silent recovery).
Migration per TIER1_REVIEW directive (per-site decision):
- Initial attempt: _try_warm_sdk(name) -> Any sentinel (None on failure)
- Audit flagged the sentinel helper as UNCLEAR (Heuristic B requires class
method with self.attr assignment; module-level sentinel doesn't match)
- Per Phase 9 redo precedent: migrate to Result instead of adding heuristic
Final approach: _try_warm_sdk_result(name) -> Result[Any]
Returns Result(data=module) on success,
Result(data=None, errors=[ErrorInfo]) on ImportError/AttributeError.
Classify callers check result.ok and use result.data on success.
Audit: ai_client SS 2 -> 0; UNCLEAR 1 -> 0 (after Result migration).
COMPLIANT 32 -> 33.
Site 11 at module level had:
if os.environ.get('SLOP_TOOL_PRESET'):
try:
set_tool_preset(os.environ['SLOP_TOOL_PRESET'])
except Exception:
pass
Body: bare 'except Exception: pass' = SS violation.
Migration: call the _set_tool_preset_result helper from Phase 11 site 5.
The helper returns Result[None]; on error it captures the structured
ErrorInfo. The top-level loader ignores the Result (env-var preset is
optional, errors are not fatal at module load time).
Audit: ai_client SS 3 -> 2.
Both sites 9 (gemini) and 10 (gemini_cli) in get_token_stats had:
try: _ensure_gemini_client()
if _gemini_client:
resp = _gemini_client.models.count_tokens(model=_model, contents=md_content)
total_tokens = cast(int, resp.total_tokens)
except Exception: pass
Body: pass = SS violation.
New helper _count_gemini_tokens_for_stats_result(md_content) -> Result[int]:
- Returns Result(data=token_count) on success
- Returns Result(data=0, errors=[ErrorInfo]) on SDK failure or warmup failure
- Caller treats 0 as 'token count unavailable' and falls back to
character-based estimation
Legacy get_token_stats now uses:
if p in ('gemini', 'gemini_cli'):
total_tokens = _count_gemini_tokens_for_stats_result(md_content).data
(combined both branches into one since the logic was identical)
Audit: ai_client SS 5 -> 3. COMPLIANT 31 -> 32.
Both functions had:
try: ToolPresetManager().load_all() ...
except (OSError, ValueError, AttributeError) as e:
sys.stderr.write(f'[ERROR] Failed to set {preset_name}: {e}')
sys.stderr.flush()
sys.stderr.write is logging = NOT a drain = SS violation per MUST-NOT-DO #6.
New helpers:
- _set_tool_preset_result(preset_name: Optional[str]) -> Result[None]
Empty/None preset short-circuits to Result(data=None).
On failure: Result(data=None, errors=[ErrorInfo]).
- _set_bias_profile_result(profile_name: Optional[str]) -> Result[None]
Same pattern.
Legacy wrappers set the global state (or skip on empty preset) and
delegate to the _result helper. Cache invalidation runs regardless.
Audit: ai_client SS 9 -> 7. COMPLIANT 27 -> 29.
Sites L432 (cleanup) and L450 (reset_session) had:
try: _gemini_client.caches.delete(name=_gemini_cache.name)
except Exception: pass
This is bare 'except: pass' = INTERNAL_SILENT_SWALLOW violation (logging is NOT
a drain; 'pass' is the worst form of silent recovery).
Migration: use existing _delete_gemini_cache_result() helper (added Phase 10).
The helper returns Result[None]; on SDK error logs a warning to comms.
The caller ignores the Result (cleanup is best-effort).
Audit: ai_client SS 11 -> 9.
All 3 run_tier4_* functions had the same pattern:
try: ... AI call ...
except Exception as e: return '[XXX FAILED] {e}' (or None)
Per TIER1_REVIEW: empty-default return = MIGRATE to Result[T].
New helpers:
- _run_tier4_analysis_result(stderr: str) -> Result[str]
Returns Result(data=analysis) on success, Result(data='', errors=[ErrorInfo])
on SDK failure. Empty stderr short-circuits to Result(data='').
- _run_tier4_patch_callback_result(stderr: str, base_dir: str) -> Result[Optional[str]]
Returns Result(data=patch) on valid diff, Result(data=None) when no
valid diff, Result(data=None, errors=[ErrorInfo]) on SDK failure.
- _run_tier4_patch_generation_result(error: str, file_context: str) -> Result[str]
Returns Result(data=patch) on success, Result(data='', errors=[ErrorInfo])
on SDK failure. Empty error short-circuits to Result(data='').
Legacy wrappers delegate to _result helpers and return result.data,
preserving original signatures (str for sites 7,9; Optional[str] for site 8).
Existing tier4 tests pass (13/13 in test_tier4_patch_generation +
test_tier4_interceptor).
Audit: ai_client BC 3 -> 0. All 9 Phase 10 BC sites migrated.
Site L1990: inner _send(r_idx) in _send_gemini_cli had:
try: resp_data = adapter.send(...)
except Exception as e: events.emit('response_received', {'error': str(e)}); raise
This is Re-Raise Pattern 2 (catch + emit event + raise). Per TIER1_REVIEW,
the migration is to Result[T] because the audit does not yet recognize
events.emit as a structured error carrier.
New helper _send_cli_round_result(r_idx, adapter, payload, ...) -> Result[dict]:
- Emits request_start + [CLI] comms before SDK call
- Returns Result(data=resp_data) on SDK success
- On failure: emits response_received error event + returns Result(errors=[ErrorInfo(original=e)])
Inner _send refactored:
send_result = _send_cli_round_result(r_idx, adapter, payload, ...)
if not send_result.ok:
raise cast(Exception, send_result.errors[0].original)
resp_data = send_result.data
This preserves the original re-raise behavior so the outer
_send_gemini_cli try/except still catches and converts to Result.
Audit: ai_client BC 4 -> 3.
Site L1773: cache.create block in _send_gemini had multiple global side
effects (sets _gemini_cache, _gemini_cache_created_at, _gemini_cached_file_paths,
returns chat_config with cached_content). Except body reset globals on failure.
Per TIER1_REVIEW: logging is NOT a drain. MIGRATE to Result[Any].
New helper _create_gemini_cache_result(sys_instr, tools_decl, file_items) -> Result[Any]:
- Returns Result(data=chat_config) on SDK success (sets globals, logs [CACHE CREATED])
- Returns Result(data=None, errors=[ErrorInfo]) on SDK failure (resets globals,
logs [CACHE FAILED])
- Preserves original semantics: globals set on success, reset on failure
Caller:
cached_config_result = _create_gemini_cache_result(sys_instr, tools_decl, file_items)
if cached_config_result.ok:
chat_config = cached_config_result.data
Audit: ai_client BC 5 -> 4. _send_gemini cache-related BC sites all migrated.
Site L1732: count_tokens block in _send_gemini had:
try: count_resp = _gemini_client.models.count_tokens(...)
... set should_cache based on total_tokens ...
except Exception as e: _append_comms('[COUNT FAILED]')
Per TIER1_REVIEW: logging is NOT a drain. MIGRATE to Result[bool].
New helper _should_cache_gemini_result(sys_instr: str) -> Result[bool]:
- Result(data=True) if token count >= 2048
- Result(data=False) if below threshold + [CACHING SKIPPED] comms note
- Result(data=False, errors=[ErrorInfo]) on SDK failure + [COUNT FAILED] comms
Caller: should_cache = _should_cache_gemini_result(sys_instr).data
Audit: ai_client BC 6 -> 5. Site L1732 (now shifted to L1752) no longer BC.
Sites L1680 (cache.delete on context change) and L1692 (cache.delete on
TTL expiry) had identical patterns:
try: _gemini_client.caches.delete(name=_gemini_cache.name)
except Exception as e: _append_comms('OUT', 'request', {'message': f'[CACHE DELETE WARN] {e}'})
Per TIER1_REVIEW: logging is NOT a drain. MIGRATE to Result[T].
Single helper _delete_gemini_cache_result() -> Result[None]:
- Returns Result(data=None) on success
- Returns Result(data=None, errors=[ErrorInfo]) on SDK failure + logs warning to comms
- Caller (_send_gemini) ignores errors (best-effort cleanup)
Audit: ai_client BC 8 -> 6. Both sites migrated.
The original function had a broken pattern: 'raise _classify_gemini_error(exc)
from exc' which raises an ErrorInfo (not an Exception) — a runtime bug.
Per TIER1_REVIEW 2026-06-20 directive: per-site decision. The body raised a
structured error carrier (ErrorInfo), but the pattern was incorrect (ErrorInfo
is not an Exception). Cleanest fix: full Result[T] migration.
New helper:
- _list_gemini_models_result(api_key: str) -> Result[list[str]]
Returns Result(data=sorted_models) on success, Result(data=[], errors=[ErrorInfo])
on SDK/network failure.
Legacy wrapper:
- _list_gemini_models(api_key: str) -> list[str]
Returns result.data (preserves original signature; callers don't see errors).
Audit: ai_client BC 9 -> 8. Site L1594 (now shifted to L1609 due to helper insertion)
no longer in INTERNAL_BROAD_CATCH.
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).
Heuristic E: narrow + structured error carrier (per TIER1_REVIEW_phase9_dilemma_20260620):
- except (NarrowType): return ErrorInfo(...) -> INTERNAL_COMPLIANT
- except (NarrowType): <item>["error"] = True -> INTERNAL_COMPLIANT
Distinguishes from the empty-default pattern (args = {}, body = ...) which
is explicitly NOT a drain per error_handling.md:528-531.
Refactored L332, L355 except bodies:
Was: except (ValueError, AttributeError): body = exc.response.text
Now: except (ValueError, AttributeError) as e: return ErrorInfo(...)
The function still returns ErrorInfo either way. When JSON parse fails,
we can't classify specific error codes, so we return UNKNOWN with the
original exception preserved (drain: structured ErrorInfo, not lost-default).
Added 2 helper methods:
_has_errorinfo_return(stmts) -> bool
_has_dict_error_true_assign(stmts) -> bool
Tests: 41 pass (28 baseline + 13 audit heuristics including the original 8).
Audit: ai_client UNCLEAR 6 -> 4 (L332+L355 now BOUNDARY_CONVERSION).
Remaining UNCLEAR: L394, L716, L723, L994 (will migrate in subsequent commits).
Was: except Exception as e (broad)
Now: except (OSError, UnicodeDecodeError) as e
The err_item drain (returned via the refreshed list with error: True flag)
is preserved. Only specific file I/O errors are caught now.
Both deepseek and minimax branches in the tool call dispatcher had:
try: args = json.loads(tool_args_str)
except: args = {}
json.JSONDecodeError is a subclass of ValueError, so narrowed to:
except (ValueError, TypeError): args = {}
This satisfies the BC classification (specific exception types).
Narrowed 3 INTERNAL_BROAD_CATCH sites to specific exception types:
1. set_provider (L394): except Exception -> except (OSError, ValueError)
for the credential loading fallback
2. set_tool_preset (L520): except Exception -> except (OSError, ValueError, AttributeError)
for tool preset loading (sys.stderr.write + flush preserved)
3. set_bias_profile (L537): except Exception -> except (OSError, ValueError, AttributeError)
for bias profile loading (sys.stderr.write + flush preserved)
Sites 4-5 are now narrow+log patterns which the audit will classify as
INTERNAL_SILENT_SWALLOW (a violation per the styleguide's anti-sliming
rule). They will be addressed in Phase 11 (silent-swallow cleanup).
The bare 'except:' in _classify_deepseek_error (L332) and _classify_minimax_error (L355)
was classified as INTERNAL_BROAD_CATCH. Narrowed to 'except (ValueError, AttributeError)'
since the only realistic exceptions from exc.response.json() are JSONDecodeError (subclass of ValueError)
and AttributeError (if exc.response is None or .json() is missing).
The TDD red moment. The implementation is renamed but the call sites
in src/, tests/, and docs still use send_result. Subsequent commits
rename the call sites and progressively move the test suite back to
green.
10 references renamed in src/ai_client.py:
- 4 'Called by: send_result' docstring tags in private provider helpers
- 1 function definition (def send_result -> def send)
- 1 [C: ...] SDM tag referencing test function names
- 2 monitor component names (start_component / end_component)
- 2 error source strings (CONFIG + INTERNAL)
Also adds scripts/tier2/apply_t1_1_edits.py - the helper script that
applied the 10 edits. Kept in scripts/tier2/ as a record of the
mechanical change pattern.
Refs: conductor/tracks/send_result_to_send_20260616/
Removes the @deprecated send() function (was at src/ai_client.py:2939-3000)
and the from typing_extensions import deprecated import (line 38). The
function is replaced by send_result() which has been the canonical public
API since the data_oriented_error_handling_20260606 track (commit 9f86b2be).
All 3 production call sites (src/conductor_tech_lead.py:68,
src/orchestrator_pm.py:86, src/multi_agent_conductor.py:591) and 18 test
files were migrated in Phases 1-2; 4 pre-existing failures were fixed in
Phases 3-4. No remaining callers of ai_client.send(.
Verification:
- uv run rg 'def send\\(' src/ai_client.py returns 0 hits
- import src.ai_client; hasattr(ai, 'send') is False
- 73/73 migrated tests pass
Adds a new wrap_reasoning_in_text: bool = False keyword argument to
run_with_tool_loop. When True and reasoning_content is non-empty, the
returned text is prepended with <thinking>...</thinking> tags so
thinking_parser.parse_thinking_trace can extract a ThinkingSegment
for the discussion entry.
The wrap is conditional (default False) so it doesn't break providers
that already wrap inline (e.g. DeepSeek, which wraps at line 2117-2118
before run_with_tool_loop sees the response).
_send_minimax now passes wrap_reasoning_in_text=bool(caps.reasoning).
When caps.reasoning is True (M2.5/M2.7), the reasoning is wrapped in
<thinking> tags. When False (M2/M2.1), the parameter is False and
no wrap happens (avoids useless getattr on non-reasoning models).
Also fixes a bug in the test_fr3_minimax_thinking_in_returned_text
test mock: it was returning a raw MagicMock instead of a Result
object, which caused the test to see auto-created MagicMock attributes
instead of the expected text. Now wraps in Result(data=MagicMock(...))
and sets ai_client._model to ensure get_capabilities('minimax', _model)
resolves to the M2.7 capabilities (reasoning=True).
This resolves the 401 Unauthorized/invalid api_id error by letting the MiniMax client default to api.minimax.io/v1 (like the model listing logic) or read a custom base_url from credentials.toml.
This resolves the issue where calling 'send_openai_compatible' discarded the NormalizedResponse details, resulting in an AttributeError when accessing 'raw_response' inside the tool loop.
ed