Repository navigation
fix(agent): coordinate truncated tool call argument repair #342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |
| from agent.model_metadata import is_local_endpoint | ||
| from agent.message_sanitization import ( | ||
| _sanitize_surrogates, | ||
| _repair_tool_call_arguments, | ||
| repair_tool_call_arguments_with_status, | ||
| ) | ||
| from tools.terminal_tool import is_persistent_env | ||
| from utils import base_url_host_matches, base_url_hostname, env_float, env_int | ||
|
|
@@ -1124,6 +1124,35 @@ def rewrite_prompt_model_identity(agent, model: str, provider: str) -> None: | |
| agent._cached_system_prompt = sp | ||
|
|
||
|
|
||
| def _fallback_entry_key(fb: dict) -> tuple[str, str, str]: | ||
| return ( | ||
| str(fb.get("provider") or "").strip().lower(), | ||
| str(fb.get("model") or "").strip(), | ||
| str(fb.get("base_url") or "").strip().rstrip("/"), | ||
| ) | ||
|
|
||
|
|
||
| def _fallback_entry_unavailable_without_network(agent, fb: dict) -> Optional[str]: | ||
| """Return a skip reason for fallback entries known to be unusable locally.""" | ||
| fb_provider = (fb.get("provider") or "").strip().lower() | ||
| if fb_provider != "nous": | ||
| return None | ||
| try: | ||
| from hermes_cli.auth import get_provider_auth_state | ||
|
|
||
| state = get_provider_auth_state("nous") or {} | ||
| except Exception as exc: | ||
| return f"nous_auth_unreadable:{type(exc).__name__}" | ||
| access_value = state.get("access_token") | ||
| refresh_value = state.get("refresh_token") | ||
| has_access = isinstance(access_value, str) and bool(access_value.strip()) | ||
| has_refresh = isinstance(refresh_value, str) and bool(refresh_value.strip()) | ||
| if not (has_access or has_refresh): | ||
| return "nous_token_missing" | ||
| return None | ||
|
|
||
|
|
||
|
|
||
| def try_activate_fallback(agent, reason: "FailoverReason | None" = None) -> bool: | ||
| """Switch to the next fallback model/provider in the chain. | ||
|
|
||
|
|
@@ -1164,10 +1193,29 @@ def try_activate_fallback(agent, reason: "FailoverReason | None" = None) -> bool | |
| return False | ||
| fb = agent._fallback_chain[agent._fallback_index] | ||
| agent._fallback_index += 1 | ||
| fb_key = _fallback_entry_key(fb) | ||
| unavailable = getattr(agent, "_unavailable_fallback_keys", None) | ||
| if unavailable is None: | ||
| unavailable = set() | ||
| agent._unavailable_fallback_keys = unavailable | ||
|
Comment on lines
+1196
to
+1200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Fallback provider unavailability cache is never invalidated (bug) The new _unavailable_fallback_keys set (chat_completion_helpers.py:1198-1200) permanently suppresses fallback providers after a single failure. Entries are added at three points: (1) when _fallback_entry_unavailable_without_network() detects missing auth tokens for 'nous' (line 1211), (2) when resolve_provider_client returns None (line 1275), and (3) when ANY exception occurs during 'nous' fallback activation (line 1478). The set is never cleared — it survives credential re-authentication, mid-session network recovery, and transient errors. This means a single transient DNS failure or network hiccup during Nous fallback activation permanently blacklists that provider for the remainder of the session. 💡 Suggestion: Only add entries for permanent failures (missing credentials, not-configured). Do not cache transient errors like ConnectionError or TimeoutError. Additionally, clear relevant entries from _unavailable_fallback_keys when credentials are refreshed via _try_refresh_nous_client_credentials or similar re-auth paths. 📋 Prompt for AI AgentsIn agent/chat_completion_helpers.py: (1) At line 1476-1478, before adding to unavailable, check if the exception is transient (isinstance(e, (ConnectionError, TimeoutError, OSError))) and skip adding in that case. (2) In run_agent.py _try_refresh_nous_client_credentials(), after a successful credential refresh, clear nous entries from agent._unavailable_fallback_keys. This ensures transient network issues and re-authentication don't permanently suppress providers. |
||
| if fb_key in unavailable: | ||
| logger.debug("Fallback skip: %s previously marked unavailable", fb_key) | ||
| return agent._try_activate_fallback(reason) | ||
| fb_provider = (fb.get("provider") or "").strip().lower() | ||
| fb_model = (fb.get("model") or "").strip() | ||
| if not fb_provider or not fb_model: | ||
| return agent._try_activate_fallback() # skip invalid, try next | ||
| return agent._try_activate_fallback(reason) # skip invalid, try next | ||
|
|
||
| local_skip_reason = _fallback_entry_unavailable_without_network(agent, fb) | ||
| if local_skip_reason: | ||
| unavailable.add(fb_key) | ||
| logger.warning( | ||
| "Fallback skip: %s/%s is not locally usable (%s); suppressing for this session", | ||
| fb_provider, | ||
| fb_model, | ||
| local_skip_reason, | ||
| ) | ||
| return agent._try_activate_fallback(reason) | ||
|
|
||
| # Skip entries that resolve to the current (provider, model) — falling | ||
| # back to the same backend that just failed loops the failure. Compare | ||
|
|
@@ -1182,7 +1230,7 @@ def try_activate_fallback(agent, reason: "FailoverReason | None" = None) -> bool | |
| "Fallback skip: chain entry %s/%s matches current provider/model", | ||
| fb_provider, fb_model, | ||
| ) | ||
| return agent._try_activate_fallback() | ||
| return agent._try_activate_fallback(reason) | ||
| if ( | ||
| fb_base_url_for_dedup | ||
| and current_base_url | ||
|
|
@@ -1193,7 +1241,7 @@ def try_activate_fallback(agent, reason: "FailoverReason | None" = None) -> bool | |
| "Fallback skip: chain entry base_url %s matches current backend", | ||
| fb_base_url_for_dedup, | ||
| ) | ||
| return agent._try_activate_fallback() | ||
| return agent._try_activate_fallback(reason) | ||
|
|
||
| # Use centralized router for client construction. | ||
| # raw_codex=True because the main agent needs direct responses.stream() | ||
|
|
@@ -1224,7 +1272,8 @@ def try_activate_fallback(agent, reason: "FailoverReason | None" = None) -> bool | |
| logger.warning( | ||
| "Fallback to %s failed: provider not configured", | ||
| fb_provider) | ||
| return agent._try_activate_fallback() # try next in chain | ||
| unavailable.add(fb_key) | ||
| return agent._try_activate_fallback(reason) # try next in chain | ||
| try: | ||
| from hermes_cli.model_normalize import normalize_model_for_provider | ||
|
|
||
|
|
@@ -1425,8 +1474,10 @@ def try_activate_fallback(agent, reason: "FailoverReason | None" = None) -> bool | |
| ) | ||
| return True | ||
| except Exception as e: | ||
| if fb_provider == "nous": | ||
| unavailable.add(fb_key) | ||
| logger.error("Failed to activate fallback %s: %s", fb_model, e) | ||
| return agent._try_activate_fallback() # try next in chain | ||
| return agent._try_activate_fallback(reason) # try next in chain | ||
|
|
||
|
|
||
|
|
||
|
|
@@ -2183,12 +2234,13 @@ def _call_chat_completions(): | |
| # commas, unclosed brackets, Python None, etc. | ||
| # Without repair, these hit the truncation handler | ||
| # and kill the session. _repair_tool_call_arguments | ||
| # returns "{}" for unrepairable args, which is far | ||
| # better than a crashed session. | ||
| repaired = _repair_tool_call_arguments(arguments, tool_name) | ||
| if repaired != "{}": | ||
| # reports unrepairable args separately from legitimate | ||
| # empty-object normalization, so only genuine failures | ||
| # hit the truncation handler. | ||
| repair = repair_tool_call_arguments_with_status(arguments, tool_name) | ||
| if repair.success: | ||
| # Successfully repaired — use the fixed args | ||
| arguments = repaired | ||
| arguments = repair.arguments | ||
| else: | ||
| # Unrepairable — flag for truncation handling | ||
| has_truncated_tool_args = True | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,7 @@ | |
| from agent.memory_manager import build_memory_context_block | ||
| from agent.message_sanitization import ( | ||
| close_interrupted_tool_sequence, | ||
| _repair_tool_call_arguments, | ||
| repair_tool_call_arguments_with_status, | ||
| _sanitize_messages_non_ascii, | ||
| _sanitize_messages_surrogates, | ||
| _sanitize_structure_non_ascii, | ||
|
|
@@ -932,10 +932,10 @@ def run_conversation( | |
| ), | ||
| }} | ||
| except Exception: | ||
| tc["function"]["arguments"] = _repair_tool_call_arguments( | ||
| tc["function"]["arguments"] = repair_tool_call_arguments_with_status( | ||
| tc["function"]["arguments"], | ||
| tc["function"].get("name", "?"), | ||
| ) | ||
| ).arguments | ||
| new_tcs.append(tc) | ||
| am["tool_calls"] = new_tcs | ||
|
|
||
|
|
@@ -2034,27 +2034,11 @@ def _perform_api_call(next_api_kwargs): | |
| api_duration, _cache_pct, | ||
| ) | ||
|
|
||
| # On the MoA path, agent.model/provider are the virtual | ||
| # preset name ("closed") and "moa", which have no pricing | ||
| # entry — estimating against them returns None and silently | ||
| # drops the aggregator's own spend, leaving the session cost | ||
| # as advisor-fan-out only (a ~50% undercount when the | ||
| # aggregator does the full acting loop). Price the aggregator | ||
| # turn at its REAL model/provider, read from the MoA client's | ||
| # resolved aggregator slot. | ||
| _agg_cost_model = agent.model | ||
| _agg_cost_provider = agent.provider | ||
| _agg_cost_base_url = agent.base_url | ||
| _agg_slot = getattr(_moa_client, "last_aggregator_slot", None) if _moa_client is not None else None | ||
| if _agg_slot and _agg_slot.get("model"): | ||
| _agg_cost_model = _agg_slot["model"] | ||
| _agg_cost_provider = _agg_slot.get("provider") or agent.provider | ||
| _agg_cost_base_url = _agg_slot.get("base_url") or agent.base_url | ||
| cost_result = estimate_usage_cost( | ||
| _agg_cost_model, | ||
| agent.model, | ||
| aggregator_usage, | ||
| provider=_agg_cost_provider, | ||
| base_url=_agg_cost_base_url, | ||
| provider=agent.provider, | ||
| base_url=agent.base_url, | ||
| api_key=getattr(agent, "api_key", ""), | ||
| ) | ||
|
Comment on lines
2037
to
2043
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MoA aggregator cost silently dropped from session cost tracking (bug) The PR removes the specialized cost-estimation path that read _moa_client.last_aggregator_slot to get the REAL model/provider of the MoA aggregator for pricing. Now conversation_loop.py:2037-2043 passes agent.model/agent.provider/agent.base_url directly to estimate_usage_cost(). On MoA paths these are virtual identifiers ('closed' + 'moa') with no pricing entry, so estimate_usage_cost() returns amount_usd=None and the aggregator's spend is silently lost from agent.session_estimated_cost_usd. The old code (with an explicit comment warning of ~50% undercount) used last_aggregator_slot to resolve the real model/provider. The slot is still populated in moa_loop.py:717 and tested in test_moa_aggregator_cost_slot.py, but is now dead production code. 💡 Suggestion: Restore the aggregator slot resolution for MoA cost estimation. Before calling estimate_usage_cost, check if _moa_client is available with a last_aggregator_slot containing the real model/provider/base_url, and use those for pricing instead of agent.model/agent.provider. 📋 Prompt for AI AgentsIn agent/conversation_loop.py, around line 2037, restore the code that resolves the aggregator's real model/provider from _moa_client.last_aggregator_slot before calling estimate_usage_cost. The removed block was: |
||
| if cost_result.amount_usd is not None: | ||
|
|
@@ -4362,7 +4346,14 @@ def _perform_api_call(next_api_kwargs): | |
| try: | ||
| json.loads(args) | ||
| except json.JSONDecodeError as e: | ||
| invalid_json_args.append((tc.function.name, str(e))) | ||
| repair = repair_tool_call_arguments_with_status( | ||
| args, | ||
| tc.function.name, | ||
| ) | ||
| if repair.success: | ||
| tc.function.arguments = repair.arguments | ||
| continue | ||
| invalid_json_args.append((tc, str(e))) | ||
|
|
||
| if invalid_json_args: | ||
| # Check if the invalid JSON is due to truncation rather | ||
|
|
@@ -4373,31 +4364,39 @@ def _perform_api_call(next_api_kwargs): | |
| # (after stripping whitespace) are cut off mid-stream. | ||
| _truncated = any( | ||
| not (tc.function.arguments or "").rstrip().endswith(("}", "]")) | ||
| for tc in assistant_message.tool_calls | ||
| if tc.function.name in {n for n, _ in invalid_json_args} | ||
| for tc, _ in invalid_json_args | ||
| ) | ||
| if _truncated: | ||
| agent._vprint( | ||
| f"{agent.log_prefix}⚠️ Truncated tool call arguments detected " | ||
| f"(finish_reason={finish_reason!r}) — refusing to execute.", | ||
| f"(finish_reason={finish_reason!r}) — returning tool error.", | ||
| force=True, | ||
| ) | ||
| agent._invalid_json_retries = 0 | ||
| agent._cleanup_task_resources(effective_task_id) | ||
| agent._persist_session(messages, conversation_history) | ||
| return { | ||
| "final_response": "Response truncated due to output length limit", | ||
| "messages": messages, | ||
| "api_calls": api_call_count, | ||
| "completed": False, | ||
| "partial": True, | ||
| "error": "Response truncated due to output length limit", | ||
| } | ||
| recovery_assistant = agent._build_assistant_message(assistant_message, finish_reason) | ||
| messages.append(recovery_assistant) | ||
| invalid_call_ids = {tc.id for tc, _ in invalid_json_args} | ||
| for tc in assistant_message.tool_calls: | ||
| if tc.id in invalid_call_ids: | ||
| tool_result = ( | ||
| "Error: Tool call arguments were truncated due to the " | ||
| "output length limit and could not be repaired. Please " | ||
| "shorten the content or split it into multiple tool calls." | ||
| ) | ||
| else: | ||
| tool_result = "Skipped: other tool call in this response had truncated arguments." | ||
| messages.append({ | ||
| "role": "tool", | ||
| "name": tc.function.name, | ||
| "tool_call_id": tc.id, | ||
| "content": tool_result, | ||
| }) | ||
| continue | ||
|
|
||
| # Track retries for invalid JSON arguments | ||
| agent._invalid_json_retries += 1 | ||
|
|
||
| tool_name, error_msg = invalid_json_args[0] | ||
| tool_name, error_msg = invalid_json_args[0][0].function.name, invalid_json_args[0][1] | ||
| agent._buffer_vprint(f"⚠️ Invalid JSON in tool call arguments for '{tool_name}': {error_msg}") | ||
|
|
||
| if agent._invalid_json_retries < 3: | ||
|
|
@@ -4415,10 +4414,10 @@ def _perform_api_call(next_api_kwargs): | |
| messages.append(recovery_assistant) | ||
|
|
||
| # Respond with tool error results for each tool call | ||
| invalid_names = {name for name, _ in invalid_json_args} | ||
| invalid_call_ids = {tc.id for tc, _ in invalid_json_args} | ||
| for tc in assistant_message.tool_calls: | ||
| if tc.function.name in invalid_names: | ||
| err = next(e for n, e in invalid_json_args if n == tc.function.name) | ||
| if tc.id in invalid_call_ids: | ||
| err = next(e for invalid_tc, e in invalid_json_args if invalid_tc.id == tc.id) | ||
| tool_result = ( | ||
| f"Error: Invalid JSON arguments. {err}. " | ||
| f"For tools with no required parameters, use an empty object: {{}}. " | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 prompt_caching.enabled=false global kill switch removed with no replacement (bug)
The PR removes the prompt_caching.enabled=false check from anthropic_prompt_cache_policy() in agent/agent_runtime_helpers.py. This was the global kill-switch that disabled cache_control marker injection across all code paths (init, model switch, fallback re-derivation). It existed as an escape hatch for strict Anthropic-compatible proxies that inject their own cache_control markers server-side — stacking Hermes' markers on top exceeds the 4-breakpoint limit and results in HTTP 400 errors. With the kill-switch removed, users who set prompt_caching.enabled: false in config.yaml will have the setting silently ignored, and their proxies will receive doubled cache_control markers. Existing tests in tests/run_agent/test_anthropic_prompt_cache_policy.py (TestPromptCachingDisabledKillSwitch class, 5 test methods) will fail because anthropic_prompt_cache_policy() no longer reads the config setting.
💡 Suggestion: Restore the prompt_caching.enabled=false kill-switch check at the top of anthropic_prompt_cache_policy() before the model/provider analysis logic. Alternatively, if the removal is intentional, delete the orphaned test class TestPromptCachingDisabledKillSwitch from test_anthropic_prompt_cache_policy.py and document the deprecation.
📋 Prompt for AI Agents
In agent/agent_runtime_helpers.py, in the anthropic_prompt_cache_policy function, restore the following code block before the model_lower = eff_model.lower() line (currently around line 1449):
Global kill switch: prompt_caching.enabled=false disables cache_control
markers on every path (init, /model switch, fallback re-derivation).
Escape hatch for strict Anthropic-compatible proxies that inject their
own markers server-side — stacking ours on top exceeds Anthropic's
4-breakpoint limit and 400s.
try:
from hermes_cli.config import load_config as _load_pc_cfg
_pc_cfg = _load_pc_cfg().get('prompt_caching', {}) or {}
if isinstance(_pc_cfg, dict) and _pc_cfg.get('enabled') is False:
return False, False
except Exception:
pass
If the removal is intentional, remove the TestPromptCachingDisabledKillSwitch class (lines 334-409) from tests/run_agent/test_anthropic_prompt_cache_policy.py instead.