fix(agent): nudge once before the housekeeping empty-follow-up shortcut - #65674
fix(agent): nudge once before the housekeeping empty-follow-up shortcut#656740xDevNinja wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Overview
- Agent fix: nudge once before the housekeeping empty-follow-up shortcut
- 137 additions, 29 deletions
Assessment
- Improves agent behavior by adding a nudge before the housekeeping shortcut
- No obvious security concerns
- Clean diff
Reviewed by Hermes Agent
|
Thanks for the focused regression fix. The premise remains present on current main: The new regression scenario matches the reported failure mode, and GitHub reports a clean merge state with all required checks passing. Automated hermes-sweeper review. |
|
Rebased onto current |
c316739 to
2eb65d2
Compare
SummarySix PRs address this empty-after-tool-response complex: #9432 handles substantive-turn fallback for #9400; #63888, #64080, and merged #64348 handle stale housekeeping state across a later substantive tool-only turn for #63860; #64496 covers kanban terminal-guard bypasses; and #65674 directly defers the #65600 housekeeping fallback until after one recovery nudge. Related pull requests
Duplicates#63888 and #64080 are substantially duplicate implementations of the #63860 invalidation; merged #64348 salvages and supersedes both. #64496 and #65674 are not duplicates: the former enforces the kanban terminal protocol, while the latter provides general housekeeping-empty recovery. Suggested consolidationKeep #65674 open with a salvage path: return its rebased head to maintainer review as the focused #65600 fix, consistent with the visible keep_open verdict. Keep #64496 open for separate kanban-specific review; #63888 and #64080 are already closed in the explicit duplicate/salvage chain ending at merged #64348, and closed #9432 requires no further action. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I9400(["issue #9400 (closed)"])
I65600(["issue #65600 (open)"])
P65674["PR #65674 (open)"]
P65674 -.->|partial| I9400
P65674 -->|best fix| I65600
class I9400 closed
class I65600 open
class P65674 open
class P65674 best
class P65674 target
click I9400 "https://github.com/NousResearch/hermes-agent/issues/9400"
click I65600 "https://github.com/NousResearch/hermes-agent/issues/65600"
click P65674 "https://github.com/NousResearch/hermes-agent/pull/65674"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 6 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 64 kB of PR diffs, 38 kB of issue/PR text, 12 kB of discussion (15 comments), 10 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
An empty completion right after a housekeeping-only tool turn was treated as "the model has nothing more to say": the prior turn's narration was recycled as the final response and the turn ended. Weak/quantized local models intermittently return a genuinely empty completion there because they choked, not because they finished — and the shortcut fired before the standard post-tool empty-response nudge ever ran, silently ending the turn with stale content and no error. Defer the shortcut until the one nudge retry is spent: the first empty follow-up gets the standard nudge (preserving the housekeeping fallback across it), and only a second consecutive empty is served the fallback content. Genuinely-done turns cost one extra API call; choked turns now recover instead of dead-ending. Fixes NousResearch#65600
2eb65d2 to
54781d0
Compare
|
Rebased onto current |
What does this PR do?
Stops the
fallback_prior_turn_contentshortcut inagent/conversation_loop.pyfrom silently ending the turn on the first empty completion after a housekeeping-only tool turn.Today that shortcut fires before the standard post-tool empty-response nudge ever runs. That's the right trade when the model is genuinely done ("You're welcome!" + memory save → empty), but weak/quantized local models intermittently return a genuinely empty completion right after a housekeeping call because they choked, not because they finished — and they never get the retry every other empty-after-tools path gets. The turn ends with recycled narration and no error, and the session sits idle (#65600's background-review repro).
The fix defers the shortcut past the single nudge retry that already exists a few lines below:
Cost in the genuinely-done case is one extra API call, which is what #7968's original suggested direction called for ("don't let the classification fully bypass retries"). No behavior change for substantive-tool turns, thinking-only responses (prefill path untouched), or the partial-stream recovery path. Unlike #64496, this isn't gated to kanban sessions — it covers the general case.
The nudge previously cleared
_last_content_with_toolsunconditionally; it now preserves it when the flag says housekeeping-only, so the second empty can still deliver the content instead of dead-ending in the empty-response exhaustion path. Clearing is kept for substantive narration (the #63860 stale-state fix is untouched — its regression test still passes).Related Issue
Fixes #65600
Type of Change
Changes Made
agent/conversation_loop.py: compute nudge availability (_prior_was_tool/_has_inline_thinking/ retry flag) before the housekeeping fallback shortcut and gate the shortcut onnot _nudge_available; in the nudge branch, preserve the fallback when it came from a housekeeping-only turn (still cleared for substantive narration).tests/run_agent/test_conversation_fallback_state.py:test_housekeeping_empty_follow_up_gets_one_nudge_before_fallback— housekeeping turn → empty (choke) → nudge → real recovery content becomes the final response. Fails on current main with exactly the reported log line andreason=fallback_prior_turn_content,api_calls=2.test_housekeeping_only_turn_still_sets_fallbackfor the deferred contract: empty → nudge → empty again → fallback content served,reason=fallback_prior_turn_content,api_calls=3.How to Test
uv run python -m pytest tests/run_agent/test_conversation_fallback_state.py -q→ 3 passed.agent/conversation_loop.pyhunk and re-run → the new test fails with the issue's exact symptom (recycled narration, 2 API calls,fallback_prior_turn_content).uv run python -m pytest tests/run_agent/ -q→ 2052 passed.tests/agent/has 127 pre-existing failures on clean main (verified identical with and without this change; the two turn-finalizer files that reference this exit reason pass).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A