fix(agent): detect thinking-timeout for reasoning models and surface actionable guidance instead of misleading file-write advice - #52272
Conversation
…actionable guidance instead of misleading file-write advice
Two-part fix:
Part 1 (classifier override at agent/error_classifier.py:720-738):
A transport disconnect on a reasoning model — even on a large session —
now routes to FailoverReason.timeout instead of context_overflow. Without
this, large-session reasoning-model disconnects route to the compression
branch and silently delete conversation history on a phantom
context-length error. The override is strictly targeted: non-reasoning
models (gpt-4o, claude-3-5-sonnet, llama-3.3-70b, etc.) still route to
context_overflow on large sessions — the existing intentional behavior
for chat models whose proxy doesn't idle-kill during prefill/generation.
Part 2 (new agent/thinking_timeout_guidance.py + integration at
agent/conversation_loop.py:3488-3567):
New is_thinking_timeout() and build_thinking_timeout_guidance() helpers.
When a known reasoning model (NVIDIA Nemotron 3 Ultra, OpenAI o1/o3,
Anthropic Opus 4.x thinking, DeepSeek R1, Qwen QwQ, xAI Grok reasoning)
hits a transport-kill on a small session (classifier says timeout
directly) or after Part 1 routes correctly (large session), the user
now sees reasoning-specific guidance with three actionable workarounds
in priority order:
1. Set providers.<provider>.models.<model>.stale_timeout_seconds: 900
in ~/.hermes/config.yaml (Hermes's built-in floor is already 600s
for known reasoning models; raise further if upstream is even
tighter).
2. Lower reasoning_budget or set reasoning_effort: medium on this
model if the provider supports it.
3. Use a smaller / faster reasoning model if the task doesn't
require deep thinking.
The new guidance takes precedence via if/elif over the existing
_is_stream_drop block, so a reasoning-model user with a transport-kill
message sees actionable advice instead of the misleading "try
execute_code with Python's open() for large files" advice (which is
correct for the unrelated large-file-write stream-drop case but
actively wrong for the thinking-timeout case).
Verified:
- 478 tests passing across 9 directly-relevant files (49 new + 429
existing, zero regressions).
- Ruff lint clean on all 4 modified/new files.
- Negative test: 6 parametrized regression guards confirm non-reasoning
models still route to context_overflow on large sessions; 4
parametrized gates confirm non-timeout classifier reasons never
trigger the guidance; 5 parametrized cases confirm non-transport
messages never trigger it.
- Regression guard: new guidance message does NOT contain
"execute_code" or "open()" — the misleading advice is fully
replaced, not appended alongside.
- Cross-vendor dual review via agy -p:
- Gemini 3.5 Flash (Medium) — passed: true, zero blockers, one
SHOULD-FIX (vprint block duplication — fixed by extracting
detection into a helper module).
- GPT-OSS 120B (Medium) — passed: true, zero blockers, two nits
(test placement — adopted at tests/agent/test_thinking_timeout_guidance.py;
primary-model capture — accepted as non-issue per Flash's nit).
Dependency note for maintainers:
This PR includes agent/reasoning_timeouts.py (the reasoning-model
allowlist module from PR NousResearch#52238) because the Layer 1 override is
load-bearing on get_reasoning_stale_timeout_floor(). After PR NousResearch#52238
lands on main, this PR's duplicate agent/reasoning_timeouts.py should
be rebased away. Either PR can land first; the other rebase is
mechanical.
Fixes NousResearch#52271.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Comprehensive two-part fix for reasoning model thinking-timeout UX. Well-structured with classifier override, detection, and guidance. Good test coverage. The guidance provides actionable workarounds in priority order.
Changes:
agent/conversation_loop.py: Thinking-timeout detection and guidance displayagent/error_classifier.py: Reasoning model disconnect classification overrideagent/thinking_timeout_guidance.py: New module with detection and guidance functionstests/agent/test_thinking_timeout_guidance.py: Test coverage
LGTM.
Reviewed by Hermes Agent
|
Salvaged and merged via #52795 (rebase-merge, your authorship preserved in git log: commit f2024cb on main as DavidMetcalfe). Cherry-picked onto current main. Verified live before merge: bug reproduced on pristine main, fix routes reasoning+disconnect+large-session to FailoverReason.timeout (no phantom compression) while leaving chat models and genuine HTTP-400 overflows untouched; 210/210 targeted tests pass. The bundled agent/reasoning_timeouts.py is the load-bearing allowlist — if #52238 lands first the duplicate is a mechanical rebase-away. Thanks for the thorough writeup and proposed fix. |
Summary
Two-part fix for issue #52271 (reasoning-model thinking-timeout UX).
Part 1 — Classifier override at
agent/error_classifier.py:720-738: A transport disconnect on a reasoning model — even on a large session — now routes toFailoverReason.timeoutinstead ofcontext_overflow. Without this, large-session reasoning-model disconnects route to the context-compression branch and silently delete conversation history on a phantom context-length error.Part 2 — Detection + guidance via new
agent/thinking_timeout_guidance.py+ integration atagent/conversation_loop.py:3488-3567: When a known reasoning model (NVIDIA Nemotron 3 Ultra, OpenAI o1/o3, Anthropic Opus 4.x thinking, DeepSeek R1, Qwen QwQ, xAI Grok reasoning) hits a transport-kill, the user sees reasoning-specific guidance with three actionable workarounds in priority order:providers.<provider>.models.<model>.stale_timeout_seconds: 900in~/.hermes/config.yaml(Hermes's built-in floor is already 600s for known reasoning models after PR feat(agent): per-reasoning-model stale-timeout floor (Nemotron 3 Ultra, OpenAI o1/o3, Opus 4.x thinking, DeepSeek R1, Qwen QwQ, xAI Grok reasoning) #52238 lands; raise further if upstream is even tighter).reasoning_budgetor setreasoning_effort: mediumon this model if the provider supports it.The new guidance takes precedence (
if/elif) over the existing_is_stream_dropblock. A reasoning-model user with a transport-kill message now sees actionable advice instead of the misleading "tryexecute_codewith Python'sopen()for large files" advice (which is correct for the unrelated large-file-write stream-drop case but actively wrong for the thinking-timeout case).Test Plan
python3 -m pytest tests/agent/test_thinking_timeout_guidance.py tests/agent/test_error_classifier.py tests/agent/test_non_stream_stale_timeout.py tests/agent/test_stream_read_timeout_floor.py tests/agent/test_local_stream_timeout.py tests/hermes_cli/test_timeouts.py tests/run_agent/test_primary_runtime_restore.py tests/run_agent/test_streaming.py tests/agent/test_model_metadata.py -q— 478 passed, zero failures, zero regressions.ruff check agent/error_classifier.py agent/conversation_loop.py agent/thinking_timeout_guidance.py agent/reasoning_timeouts.py tests/agent/test_thinking_timeout_guidance.py— clean.agy -p:passed: true, zero blockers, zero should-fix, zero nits (after Round 1 BLOCKER fix).passed: true, zero blockers, zero should-fix, one nit (test placement — adopted attests/agent/test_thinking_timeout_guidance.py).gpt-4o,claude-3-5-sonnet-20240620,llama-3.3-70b-instruct,qwen2-72b-instruct,x-ai/grok-3,olmo-1) still route tocontext_overflowon large sessions. 4 parametrized gates confirm non-timeout classifier reasons (billing, rate_limit, auth, context_overflow, format_error, provider_policy_blocked, content_policy_blocked, thinking_signature, unknown) never trigger the guidance. 5 parametrized cases confirm non-transport messages (insufficient credits, rate limit exceeded, invalid API key, context length exceeded, tool call argument malformed) never trigger it.error_classifier.py:718-732is the load-bearing bug. Gemini Flash's Round 1 review caught it: WITHOUT the classifier override, reasoning-model users on long sessions get routed to the context-compression branch atconversation_loop.py:3037-3149, which calls_compress_context()3 times, then returns "Context length exceeded: max compression attempts (3) reached" — silently deleting conversation history on a phantom overflow. The Part 2 guidance never fires because the error never reaches the terminal site.Notes
agent/reasoning_timeouts.py(the reasoning-model allowlist module) because the Layer 1 override is load-bearing onget_reasoning_stale_timeout_floor(). After PR feat(agent): per-reasoning-model stale-timeout floor (Nemotron 3 Ultra, OpenAI o1/o3, Opus 4.x thinking, DeepSeek R1, Qwen QwQ, xAI Grok reasoning) #52238 lands onmain, this PR's duplicateagent/reasoning_timeouts.pyshould be rebased away — the maintainer can choose whichever PR lands first; the other rebase is mechanical (drop the duplicate file from this PR's diff).gpt-4o,claude-3-5-sonnet,llama-3.3-70b,qwen2-72b,x-ai/grok-3,olmo-1all still route tocontext_overflowon large sessions, exactly as before. The override is strictly targeted at reasoning models in the allowlist.providers.<id>.models.<model>.stale_timeout_secondsexplicitly. The new guidance message refers users to that existing knob..envvariables. Per project policy (.envis for secrets only), all behavioral settings live inconfig.yaml. The guidance message points users at the rightconfig.yamlkey.What this PR does NOT change
BrokenPipeError skips connection-pool rebuild in transport-recovery gate) — already open as PR fix(agent): rebuild connection pool for socket-layer transport errors (BrokenPipe, ConnectionReset, etc.) #52226.ProviderProfileschema check.Fixes #52271.