From c5c10dc0509e0f58b13c82e04dcea285029fc21e Mon Sep 17 00:00:00 2001 From: Wesley Simplicio Date: Sat, 9 May 2026 12:00:35 -0300 Subject: [PATCH 1/3] fix(session_search): use original session ID for content retrieval When FTS5 matches a delegation/compression child session, the old code overwrote result["session_id"] with the resolved parent ID and then called get_messages_as_conversation(parent_id). The parent often doesn't contain the matched text, so the summary was generated from wrong messages. Fix: store the child session ID in a new internal field `content_session_id` so content retrieval uses the exact session where the FTS5 hit lives, while `session_id` still surfaces the canonical parent ID for display and dedup. Adds regression test that fails without the fix. Closes #22150 --- tests/tools/test_session_search.py | 79 ++++++++++++++++++++++++++++++ tools/session_search_tool.py | 10 +++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_session_search.py b/tests/tools/test_session_search.py index 468a492ad8e00..093c6e34e7222 100644 --- a/tests/tools/test_session_search.py +++ b/tests/tools/test_session_search.py @@ -560,3 +560,82 @@ def _get_session(session_id): assert entry["source"] == "api_server", ( f"source should be parent's 'api_server', got {entry['source']!r}" ) + + def test_content_fetched_from_child_when_match_came_from_child(self): + """Regression for #22150: when FTS5 hits a delegation/compression child + session, content for summarization must come from that child — not the + resolved parent — because the matched text only lives in the child. + """ + from unittest.mock import MagicMock, AsyncMock, patch as _patch + from tools.session_search_tool import session_search + + mock_db = MagicMock() + mock_db.search_messages.return_value = [ + { + "session_id": "child_sid", + "content": "discussion about headless-chatgpt setup", + "source": "cli", + "session_started": 1709400000, + "model": "gpt-4o-mini", + }, + ] + + def _get_session(session_id): + if session_id == "child_sid": + return { + "id": "child_sid", + "parent_session_id": "parent_sid", + "source": "cli", + "started_at": 1709400000, + "model": "gpt-4o-mini", + } + if session_id == "parent_sid": + return { + "id": "parent_sid", + "parent_session_id": None, + "source": "cli", + "started_at": 1709300000, + "model": "gpt-4o-mini", + } + return None + + mock_db.get_session.side_effect = _get_session + + def _messages_for(sid): + if sid == "child_sid": + return [ + {"role": "user", "content": "let's set up headless-chatgpt"}, + {"role": "assistant", "content": "ok, here's how"}, + ] + if sid == "parent_sid": + return [ + {"role": "user", "content": "delegated a task"}, + {"role": "assistant", "content": "result delivered"}, + ] + return [] + + mock_db.get_messages_as_conversation.side_effect = _messages_for + + with _patch( + "tools.session_search_tool.async_call_llm", + new_callable=AsyncMock, + side_effect=RuntimeError("no provider"), + ): + result = json.loads( + session_search(query="headless-chatgpt", db=mock_db) + ) + + called_sids = [c.args[0] for c in mock_db.get_messages_as_conversation.call_args_list] + assert "child_sid" in called_sids, ( + f"content retrieval must use the child session where the match lives, " + f"got calls for: {called_sids}" + ) + assert "parent_sid" not in called_sids, ( + f"content retrieval must NOT fetch parent's messages, got calls for: {called_sids}" + ) + + assert result["success"] is True + assert result["count"] == 1 + entry = result["results"][0] + assert entry["session_id"] == "parent_sid" + assert "headless-chatgpt" in entry["summary"] diff --git a/tools/session_search_tool.py b/tools/session_search_tool.py index 2237a0cda9515..c95a421b01fb2 100644 --- a/tools/session_search_tool.py +++ b/tools/session_search_tool.py @@ -427,7 +427,14 @@ def _resolve_to_parent(session_id: str) -> str: continue if resolved_sid not in seen_sessions: result = dict(result) + # Surface the resolved parent ID for display/dedup so callers + # see the canonical conversation, but remember the child where + # the FTS5 hit actually lives for content retrieval. Without + # this, get_messages_as_conversation() below would fetch the + # parent's messages — which often don't contain the matched + # text when the hit comes from a delegation/compression child. result["session_id"] = resolved_sid + result["content_session_id"] = raw_sid seen_sessions[resolved_sid] = result if len(seen_sessions) >= limit: break @@ -436,7 +443,8 @@ def _resolve_to_parent(session_id: str) -> str: tasks = [] for session_id, match_info in seen_sessions.items(): try: - messages = db.get_messages_as_conversation(session_id) + content_sid = match_info.get("content_session_id", session_id) + messages = db.get_messages_as_conversation(content_sid) if not messages: continue session_meta = db.get_session(session_id) or {} From 2518c90d9dc8ae050e9e29ca9d51cc6900e3e0a9 Mon Sep 17 00:00:00 2001 From: Wesley Simplicio Date: Sat, 9 May 2026 12:46:37 -0300 Subject: [PATCH 2/3] fix(error_classifier,fallback): classify custom-provider timeouts correctly, prevent self-selection fallback loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs reported in #22548: 1. RuntimeError("claude CLI turn timed out") from a local OpenAI-compatible shim was classified as FailoverReason.unknown because the type name is not in _TRANSPORT_ERROR_TYPES and is not a subclass of TimeoutError/ConnectionError. The retry loop then treated the empty HTTP response as a genuine empty model response, triggering empty-response retries and emitting misleading "Empty response from model" warnings. Fix: add _TIMEOUT_MESSAGE_PATTERNS to _classify_by_message() so message strings containing "timed out", "deadline exceeded", "request timed out", etc. are classified as FailoverReason.timeout regardless of the exception type. 2. When the fallback chain contained the same custom provider endpoint that just failed (e.g. two entries both pointing to http://127.0.0.1:7891/v1 with the same model), _try_activate_fallback() would switch to it and create a retry loop against the same timed-out surface. Fix: add a self-selection guard in _try_activate_fallback() — if the resolved fallback base_url and model match the currently active provider, skip that entry and recurse to the next in the chain. Adds 3 regression tests covering RuntimeError with "timed out", "request timed out", and "deadline exceeded" message patterns. Closes #22548 --- agent/error_classifier.py | 22 +++++++++++++++++++ run_agent.py | 33 ++++++++++++++++++++++++++++ tests/agent/test_error_classifier.py | 22 +++++++++++++++++++ 3 files changed, 77 insertions(+) diff --git a/agent/error_classifier.py b/agent/error_classifier.py index 419a984b75e6e..1a42a9589eee2 100644 --- a/agent/error_classifier.py +++ b/agent/error_classifier.py @@ -254,6 +254,20 @@ def is_auth(self) -> bool: "signature", # Combined with "thinking" check ] +# Message-string patterns that indicate a provider-side timeout even when +# the exception type is generic (e.g. RuntimeError from a local shim that +# wraps a subprocess timeout). Checked before the type-based transport +# heuristics so custom-provider "timed out" errors don't fall through to +# the unknown bucket and get misreported as empty responses. +_TIMEOUT_MESSAGE_PATTERNS = [ + "timed out", + "turn timed out", + "request timed out", + "deadline exceeded", + "operation timed out", + "upstream timed out", +] + # Transport error type names _TRANSPORT_ERROR_TYPES = frozenset({ "ReadTimeout", "ConnectTimeout", "PoolTimeout", @@ -963,6 +977,14 @@ def _classify_by_message( should_fallback=True, ) + # Timeout message patterns — generic exception types (e.g. RuntimeError) + # raised by local shims or custom providers that internally wrap a + # subprocess/HTTP timeout. Classified as transport timeout so the retry + # loop rebuilds the client instead of treating the turn as an empty + # model response. + if any(p in error_msg for p in _TIMEOUT_MESSAGE_PATTERNS): + return result_fn(FailoverReason.timeout, retryable=True) + return None diff --git a/run_agent.py b/run_agent.py index 801678f371e5d..d1871de3046ff 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3529,6 +3529,19 @@ def _extract_reasoning(self, assistant_message) -> Optional[str]: # instead of returning structured reasoning fields. Only fall back # to inline extraction when no structured reasoning was found. content = getattr(assistant_message, "content", None) + if not reasoning_parts and isinstance(content, list): + # DeepSeek V4 Pro (and compatible providers) return content as a + # list of typed blocks, e.g.: + # [{"type": "thinking", "thinking": "..."}, {"type": "output", ...}] + # Without this branch the thinking text is silently dropped and the + # next turn fails with HTTP 400 ("thinking must be passed back"). + # Refs #21944. + for block in content: + if isinstance(block, dict) and block.get("type") == "thinking": + thinking_text = block.get("thinking") or block.get("text") or "" + thinking_text = thinking_text.strip() + if thinking_text and thinking_text not in reasoning_parts: + reasoning_parts.append(thinking_text) if not reasoning_parts and isinstance(content, str) and content: inline_patterns = ( r"(.*?)", @@ -8067,6 +8080,26 @@ def _try_activate_fallback(self, reason: "FailoverReason | None" = None) -> bool # Determine api_mode from provider / base URL / model fb_api_mode = "chat_completions" fb_base_url = str(fb_client.base_url) + + # Self-selection guard: skip this fallback entry when its resolved + # base_url + model are identical to the currently active provider. + # Prevents custom-provider timeout loops where the fallback chain + # re-selects the same local shim endpoint that just failed (#22548). + _current_base_url = str(getattr(self, "base_url", "") or "").rstrip("/") + _fb_base_url_norm = fb_base_url.rstrip("/") + if ( + _fb_base_url_norm + and _current_base_url + and _fb_base_url_norm == _current_base_url + and fb_model == (getattr(self, "model", "") or "") + ): + logging.warning( + "Skipping fallback to %s/%s — same endpoint/model as current " + "provider; would loop. Trying next in chain.", + fb_provider, fb_model, + ) + return self._try_activate_fallback(reason=reason) + _fb_is_azure = self._is_azure_openai_url(fb_base_url) if fb_provider == "openai-codex": fb_api_mode = "codex_responses" diff --git a/tests/agent/test_error_classifier.py b/tests/agent/test_error_classifier.py index d3f62c847c700..a6fb56a70752c 100644 --- a/tests/agent/test_error_classifier.py +++ b/tests/agent/test_error_classifier.py @@ -587,6 +587,28 @@ def test_timeout_error_builtin(self): result = classify_api_error(e) assert result.reason == FailoverReason.timeout + def test_runtime_error_cli_turn_timed_out_classifies_as_timeout(self): + # RuntimeError from a local claude-cli shim that wraps a subprocess + # timeout must classify as FailoverReason.timeout, not unknown, so + # the retry loop rebuilds the client instead of treating the turn as + # an empty model response (#22548). + e = RuntimeError("claude CLI turn timed out") + result = classify_api_error(e) + assert result.reason == FailoverReason.timeout + assert result.retryable is True + + def test_runtime_error_request_timed_out_classifies_as_timeout(self): + e = RuntimeError("request timed out after 120s") + result = classify_api_error(e) + assert result.reason == FailoverReason.timeout + assert result.retryable is True + + def test_runtime_error_deadline_exceeded_classifies_as_timeout(self): + e = RuntimeError("deadline exceeded") + result = classify_api_error(e) + assert result.reason == FailoverReason.timeout + assert result.retryable is True + # ── Error code classification ── def test_error_code_resource_exhausted(self): From e22e2ce7102fe421fbb77e6d3cc58926fd41c731 Mon Sep 17 00:00:00 2001 From: Wesley Simplicio Date: Sat, 9 May 2026 15:26:26 -0300 Subject: [PATCH 3/3] fix(fallback): use resolved_fb_model from router for self-selection guard resolve_provider_client may adjust or drop an incompatible model override (e.g. under provider: auto). Use the resolved value for both the self-selection comparison and the eventual self.model assignment so api_mode detection and downstream requests stay consistent. --- run_agent.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/run_agent.py b/run_agent.py index d1871de3046ff..79f6855c7fc2d 100644 --- a/run_agent.py +++ b/run_agent.py @@ -8070,6 +8070,11 @@ def _try_activate_fallback(self, reason: "FailoverReason | None" = None) -> bool "Fallback to %s failed: provider not configured", fb_provider) return self._try_activate_fallback() # try next in chain + # Use the model the router actually resolved to (it may drop or + # adjust an incompatible override under provider: auto). Fall back + # to the configured value only if the router returned None. + if _resolved_fb_model: + fb_model = _resolved_fb_model try: from hermes_cli.model_normalize import normalize_model_for_provider