fix(agent): allow bounded repeated stall retries - #35642
Conversation
dflash (Qwen3.6-27B Q4_K_M, lucebox spec-decode) sometimes ends an agentic
decision turn with EOS right after a short action preamble ("Let me check
X:") and NO tool_call, stalling the loop. Higher-precision weights (the
stock qwen3.6-27b-256k lane on the same host) continue to a real tool call
on the identical prompt.
This adds agent/stall_retry.py: when a no-tool-call turn looks like that
stall (short, announces an action, not a genuine completion) and
HERMES_STALL_RETRY_MODEL is set, re-issue the SAME turn once on that lane;
if it yields tool calls, adopt it and continue. Same provider/endpoint so
only the model name is overridden (no client rebuild). Fires at most once
per conversation. No-op unless the env is set, so default behavior is
unchanged.
Validated: detector 17/17 on real captured stall contents, 0 false
positives on genuine completions; live retry recovered real stalls into
real tool calls (terminal/execute_code) against the live endpoint; runs
correctly under `hermes -z` (probe-confirmed, env propagated).
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Reviewed Changes
- agent/stall_retry.py (new) — dflash Q4 premature-EOS detection and retry against a higher-quality lane.
- agent/conversation_loop.py — Integration point: bounded per-turn retry count (
HERMES_STALL_RETRY_MAX_PER_TURN, default 5), retry logic inserted before the final-response branch. - tests/agent/test_stall_retry.py — 8 tests: action preamble detection, completion distinction, incomplete fragments, retry model switching, loop integration assertions.
✅ Looks Good
- Correctness: The stall detection in
looks_like_stall()is well-calibrated — it distinguishes action preambles (Let me check X:) from genuine completions (Done.,nothing to do). The action regex covers 12+ English patterns. The completion regex covers 15+ patterns. The incomplete-final-fragment heuristic (≥80 chars, no natural ending) catches dflash truncation mid-sentence. - Opt-in design: Entirely gated on
HERMES_STALL_RETRY_MODEL— zero behavior change by default. - Safety: Retry is bounded per turn (default 5, env-configurable). The retry uses non-streaming for simplicity. Any exception during retry returns
None, causing no harm. If the retry model equals the original, it short-circuits. - Test coverage: 8 unit tests covering stall detection, completion distinction, short answers, incomplete fragments, retry model switching, and loop integration. The
source.indexOfpattern in the loop integration tests is a bit brittle but pragmatic for ensuring retry happens before tool-call check. - Documentation: The new
stall_retry.pyhas an excellent module docstring explaining the problem, the solution, env variables, and design rationale.
Reviewed by Hermes Agent
|
Pushed follow-up commit Validation:
The local fork mirror for deployment tracking is OmarB97#51. |
|
Follow-up pushed in f981551:
Verified locally:
Also deployed the same Python fix to the live taro Hermes install and verified the retry lane canary recovered through the configured |
|
Follow-up pushed in 0b86506 after live phone reproduction: Root cause of the latest 401 screenshot was secondary fallback, not primary auth: local This commit fixes the systemic retry-config issue that let that happen: an empty per-agent Verification:
|
|
Follow-up from live phone evidence at 2026-05-31 16:00 PDT: The latest abrupt stop was a short visible final after a tool result, not a 401/auth failure and not an empty response: Hermes accepted that as Updated this PR with
Verification:
Replacement fork mirror because #53 was already merged: OmarB97#54 |
|
Updated this PR with the post-#56 dflash stability work. What changed:
Verification:
Live taro evidence after fork mirror #56 merged:
Fork mirror tracking:
|
|
Follow-up pushed: the latest live phone trace showed repeated successful qwen rescues being counted as retry-budget exhaustion, and the next became slow because Hermes kept bouncing dflash -> qwen -> dflash with ~50k+ context.\n\nChanges now on this PR:\n- recovered retry-lane tool calls no longer consume the terminal stall budget; only unrecovered retry failures do\n- after repeated successful rescues (, default 2), Hermes promotes the configured retry lane for the rest of that user turn\n- the promotion is turn-scoped: normal primary runtime restoration brings dflash back on the next user turn\n- local telemetry records / events for postmortems\n\nVerification:\n- ..................................... [100%] |
|
Follow-up pushed: the latest live phone trace showed repeated successful qwen rescues being counted as retry-budget exhaustion, and the next Changes now on this PR:
Verification:
Fresh fork mirror PR for this follow-up: OmarB97#57 |
|
Updated this upstream stack with the latest dflash phone-session recovery fix. Root cause addressed: after the dflash retry lane is promoted for the current turn, the promoted lane can itself return an action preamble with no tool call. The previous state machine treated that as terminal, so long mobile/TUI sessions could still stop even after recovery promotion. Added in commit
Validation run on the upstream-stack branch:
Also deployed the equivalent fork-main commit to taro live Hermes and restarted |
|
Follow-up pushed in What changed:
Verification:
Live/taro evidence from the fork mirror path: the canary reproduced the old no-first-chunk dflash stall, then after the change it skipped same-primary recovery and went straight to fallback; aggressive stress settings now fail as structured nonzero exits instead of hanging indefinitely. |
|
Added one more commit, The earlier Additional verification after this commit:
|
|
Update after live hardening loop:
Verification:
|
|
Follow-up pushed: fixed the Windows-footgun CI failure in the dflash diagnostics scripts. The runtime checks already guarded POSIX-only signals, but the static checker requires checker-visible guarded patterns, so this replaces bare signal references with getattr-guarded values and avoids os.kill(pid, 0).\n\nVerified locally:\n- python3 scripts/check-windows-footguns.py --all\n- python3 -m py_compile hermes_cli/oneshot.py scripts/recover_local_llama_server.py scripts/dflash_stability_canary.py\n- scripts/run_tests.sh tests/agent/test_local_stream_timeout.py tests/agent/test_local_backend_recovery.py tests/agent/transports/test_chat_completions.py tests/agent/test_stall_retry.py tests/scripts/test_dflash_stability_canary.py (178 passed) |
|
Follow-up pushed for CI: updated the stale timeout test that still expected implicit local non-stream calls to disable the stale detector. That old expectation is the failure mode this PR fixes, so the test now asserts the new finite 120s local default and documents the local-backend default path.\n\nVerified locally after the change:\n- python3 -m py_compile run_agent.py tests/hermes_cli/test_timeouts.py\n- scripts/run_tests.sh tests/hermes_cli/test_timeouts.py tests/agent/test_local_stream_timeout.py tests/agent/test_local_backend_recovery.py tests/agent/transports/test_chat_completions.py tests/agent/test_stall_retry.py tests/scripts/test_dflash_stability_canary.py (190 passed in this upstream worktree) |
|
Follow-up pushed from the live hardening loop: cycle 5 exposed a marker-mismatch where the onboard canary returned the status canary marker. I hardened the canary runner so every case invocation uses a unique nonce marker, which prevents static marker echo/cross-talk from being mistaken for a valid run.\n\nVerified locally:\n- python3 scripts/check-windows-footguns.py scripts/dflash_stability_canary.py scripts/dflash_hardening_loop.py tests/scripts/test_dflash_stability_canary.py\n- scripts/run_tests.sh tests/scripts/test_dflash_stability_canary.py tests/agent/test_local_stream_timeout.py tests/agent/test_local_backend_recovery.py tests/agent/transports/test_chat_completions.py tests/agent/test_stall_retry.py tests/hermes_cli/test_timeouts.py (192 passed in this upstream worktree)\n\nAlso deployed this to taro and restarted the dflash-hardening tmux loop. |
|
Follow-up pushed from the next hardening-loop failure: after nonce markers, the short-fragment canary failed because the prompt said 'current Hermes checkout' while the canary cwd is the MeshBoard workspace. That made the canary probe the wrong filesystem location. I changed the canary to inject the actual Hermes source root into the prompt.\n\nVerified locally:\n- python3 -m py_compile scripts/dflash_stability_canary.py tests/scripts/test_dflash_stability_canary.py\n- python3 scripts/check-windows-footguns.py scripts/dflash_stability_canary.py tests/scripts/test_dflash_stability_canary.py\n- scripts/run_tests.sh tests/scripts/test_dflash_stability_canary.py (11 passed)\n\nDeployed to taro and restarted dflash-hardening again. |
|
Latest status after the source-root canary follow-up:\n\n- Fork PR #62 checks are green, including all 6 test shards, e2e, nix, ruff/ty, and Windows footguns. This upstream mirror PR currently reports no upstream status checks on the fork head branch.\n- The deployed taro dflash-hardening loop passed two fresh cycles with nonce markers and the corrected Hermes source-root prompt: evidence .\n\nI am leaving the MeshBoard tasks in review, not closed, until the relevant PRs are merged/closed and the task contract remains satisfied after settlement. |
|
Correction to the previous status comment: the live taro evidence path is /home/omar/.hermes/logs/dflash-stability-canary/20260601T063707Z.jsonl. That log shows two fresh green dflash-hardening cycles with nonce markers and the corrected Hermes source-root prompt. |
|
Follow-up pushed from cycle-3 hardening evidence: the short-fragment canary prompt was still describing the old single-argument shape. It now probes the real API: looks_like_incomplete_final_fragment(content, "stop", False, 400). Verified with py_compile plus scripts/run_tests.sh tests/scripts/test_dflash_stability_canary.py tests/agent/test_stall_retry.py (47 passed). Deployed to taro and restarted dflash-hardening again. |
|
Latest status: fork PR #62 checks are fully green after the final canary prompt fix. The upstream mirror PR head is updated to the same content; GitHub reports no upstream status checks on this fork head. The taro dflash-hardening loop passed three fresh full cycles on evidence /home/omar/.hermes/logs/dflash-stability-canary/20260601T064421Z.jsonl. Leaving MeshBoard tasks in review until PR settlement and post-settlement verification. |
|
Split follow-up from
I left this source PR open rather than closing it automatically, but it should be treated as a snowball reference branch, not the merge target. The smaller PRs are the reviewable path forward. |
3 participants
Summary
HERMES_STALL_RETRY_MAX_PER_TURNwith a default of 5 retries per user turn.20260530_193349_ae35ee.Root Cause
The previous retry guard used a single
_stall_retry_usedboolean for the whole user turn. In the new repro, the first dflash stall was correctly retried on Q6 and recovered into a real terminal tool call. After that tool result, the next dflash decision stalled again, but the boolean had already disabled recovery, so Hermes acceptedLet me look at open tasks...as the final answer.Stacking Note
This PR is stacked on #35638, which is itself stacked on #35620. The new commit in this stack is
39fcecb16 fix(agent): allow bounded repeated stall retries; once the earlier dflash retry PRs land, this should collapse to the bounded retry-counter change and tests.Validation
scripts/run_tests.sh tests/agent/test_stall_retry.pyImpact
Long dflash agentic turns can now recover from multiple independent no-tool-call stalls after tool progress, while still having a finite per-turn cap to prevent retry loops.