fix(agent): coordinate truncated tool call argument repair - #342
hashbender wants to merge 1 commit into
Conversation
|
Review Complete Files Reviewed: 8 By Severity:
This PR introduces three high-severity regressions and one medium-severity defect: MoA session cost tracking silently dropped (~50% undercount), the prompt_caching.enabled=false kill switch removed (breaking strict Anthropic proxies with HTTP 400 errors), and a Vertex AI token refresh path that imports a non-existent module (dead recovery code). Files Reviewed (8 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (72/100) — 3 high findings, 1 medium · 389 LOC across 8 files
Critical Regressions
MoA aggregator cost tracking broken (agent/conversation_loop.py:2037-2043) — The specialized cost-estimation path that resolved the real model/provider from _moa_client.last_aggregator_slot was removed. estimate_usage_cost() now receives virtual identifiers 'closed' + 'moa' with no pricing entry, causing ~50% undercount of session costs. The last_aggregator_slot is still populated in moa_loop.py:717 but is now dead production code. Orphaned test test_moa_aggregator_cost_slot.py documents the expected behavior.
prompt_caching.enabled=false kill switch removed (agent/agent_runtime_helpers.py:1444-1447) — The global escape hatch for strict Anthropic-compatible proxies was deleted. Users who set prompt_caching.enabled: false to prevent doubled cache_control markers will now hit the 4-breakpoint limit and receive HTTP 400 errors. Five test methods in TestPromptCachingDisabledKillSwitch will fail on merge.
Vertex AI credential refresh is dead code (run_agent.py:4231) — _try_refresh_vertex_client_credentials() imports get_vertex_config from agent.vertex_adapter, but agent/vertex_adapter.py does not exist. The import is wrapped in try/except, so the method silently returns False every time. Long-lived gateway sessions will experience unrecoverable 401 errors after ~1 hour of token expiry.
Related Issue
Fallback provider blacklist never invalidated (agent/chat_completion_helpers.py:1196-1200) — _unavailable_fallback_keys is populated on failure but never cleared. A transient DNS error permanently blacklists a fallback provider for the session. This set also survives credential re-authentication via _try_refresh_nous_client_credentials.
| 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", ""), | ||
| ) |
There was a problem hiding this comment.
🟠 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 Agents
In 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:
_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
Then use _agg_cost_model, _agg_cost_provider, and _agg_cost_base_url in the estimate_usage_cost call instead of agent.model, agent.provider, and agent.base_url.
| except Exception: | ||
| pass | ||
|
|
||
| model_lower = eff_model.lower() |
There was a problem hiding this comment.
🟠 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.
| from agent.vertex_adapter import get_vertex_config | ||
|
|
||
| token, base_url = get_vertex_config() |
There was a problem hiding this comment.
🟠 Vertex AI OAuth2 token refresh is dead code — agent.vertex_adapter module missing (bug)
_try_refresh_vertex_client_credentials() in run_agent.py:4217-4252 imports get_vertex_config from agent.vertex_adapter, but agent/vertex_adapter.py does not exist on disk. The import is wrapped in try/except Exception, so the method silently returns False on every invocation. This means the Vertex credential refresh path added in this PR can never succeed. When a Vertex bearer token expires (~1h), the 401 retry handler at conversation_loop.py:2601-2610 will call this method, get False, and fall through to error handling without recovering. The same missing module also affects resolve_provider_client in auxiliary_client.py:4541, which already handles ImportError gracefully. The new method compounds the problem by offering a false sense of recovery.
💡 Suggestion: Create agent/vertex_adapter.py with get_vertex_config() and has_vertex_credentials() functions. get_vertex_config() should use google.auth.default() to obtain an OAuth2 access token for Vertex AI and construct the OpenAI-compatible base_url. If google-auth is an optional dependency, ensure the ImportError handling gracefully degrades.
📋 Prompt for AI Agents
Create agent/vertex_adapter.py with get_vertex_config() and has_vertex_credentials() functions. get_vertex_config() should use google.auth.default() or google.auth.transport.requests to obtain an OAuth2 access token for the Vertex AI scope (https://www.googleapis.com/auth/cloud-platform) and construct the OpenAI-compatible base_url from the project ID and region. has_vertex_credentials() should return True when GOOGLE_APPLICATION_CREDENTIALS is set or default credentials are available. If the module is intentionally omitted (e.g., as an optional dependency), update the docstring in run_agent.py to explain the conditional availability.
| fb_key = _fallback_entry_key(fb) | ||
| unavailable = getattr(agent, "_unavailable_fallback_keys", None) | ||
| if unavailable is None: | ||
| unavailable = set() | ||
| agent._unavailable_fallback_keys = unavailable |
There was a problem hiding this comment.
🟡 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 Agents
In 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.
1 participant
What does this PR do?
Coordinates truncated tool-call argument handling around one repair contract: repairable malformed JSON continues normally, while unrepairable truncated JSON is reported back to the model as a tool error instead of executing with
{}or aborting prematurely.Related Issue
Type of Change
Shared root cause
agent/message_sanitization.pyreturned only repaired argument text, so callers could not distinguish a legitimate empty{}from the last-resort{}used after failed repair.agent/conversation_loop.pyhad a separate invalid-JSON truncation branch that rejected router-mislabeled truncated tool calls before trying the shared repair routine.How this fixes each issue
Changes Made
repair_tool_call_arguments_with_status()so callers can tell whether repair succeeded even when the returned JSON is{}.How to Test
HERMES_HOME=/private/tmp/hermes-test-home /opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/run_agent/test_repair_tool_call_arguments.py tests/run_agent/test_tool_call_args_sanitizer.py tests/run_agent/test_run_agent.py -q -x --timeout=60'BASE=$(git merge-base origin/main HEAD); { git diff --name-only --diff-filter=d "$BASE"; git ls-files --others --exclude-standard; } | grep -E '\.pyi?$' | sort -u | xargs ruff checkpython scripts/check-windows-footguns.py run_agent.py agent/message_sanitization.py agent/agent_runtime_helpers.py agent/chat_completion_helpers.py agent/conversation_loop.py tests/run_agent/test_repair_tool_call_arguments.py tests/run_agent/test_tool_call_args_sanitizer.py tests/run_agent/test_run_agent.py/opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' sh; it aborted during collection becausefastapi/uvicornare not installed and this Homebrew Python is externally managed, so the lazy dependency installer cannot install them.What platforms tested on
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass; broad collection is blocked locally by missing dashboard dependencies as noted aboveDocumentation & Housekeeping
cli-config.yaml.exampleupdate N/ACONTRIBUTING.md/AGENTS.mdupdate N/AScreenshots / Logs
run_agent.py:107# noqadirective format)Refs NousResearch#35151
Refs NousResearch#35574
Mirror-of: NousResearch#56399
NousResearch#56399