fix(acp): cancelled turn with final_response=None permanently bricks the session (every prompt queued forever) - #64471
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Fix for ACP cancelled turns with final_response=None causing permanent blocking. Substantial fix with 170 additions, 78 deletions. No security concerns.
Reviewed by Hermes Agent
|
Thanks for the targeted ACP regression fix. The current-main premise is verified: The PR normalizes the value before that prefix check and places the post-turn release in This is an automated hermes-sweeper review. |
|
Confirmed on my end. Running Hermes ACP with local Ollama model (qwen3.6-27B) via Zed agent panel. Reproduction:
This has been blocking my workflow consistently. The fix in this PR looks correct — wrapping post-turn tail in try/finally and normalizing None before Environment: Hermes latest main, Zed stable, Linux, local Ollama at http://192.168.1.15:9090/v1 |
…ponse=None
A /stop (session/cancel) landing mid tool call makes run_conversation
return {final_response: None, interrupted: True} — the key is
PRESENT, so result.get("final_response", "") keeps the None and
None.startswith(INTERRUPT_WAITING_FOR_MODEL_PREFIX) raises
AttributeError. The exception escaped prompt() BEFORE
state.is_running was reset, permanently marking the ACP session busy:
every subsequent user message hit the is_running guard and was parked
as "Queued for the next turn. (N queued)" with no turn ever draining
the queue.
Two-layer fix:
- coerce final_response with "or empty-string" so None cannot reach .startswith
- wrap the whole post-turn tail (persist, provenance update,
auto-title, final-response delivery) in try/finally that guarantees
the is_running/current_prompt_text reset, so no future tail
exception can brick the session again
Regression tests: cancelled turn with final_response=None releases the
session and the follow-up prompt runs; a forced tail exception still
resets is_running.
650029f to
7daec52
Compare
Problem
Cancelling an ACP turn (
session/cancel, e.g. hitting Stop in Zed / a VS Code ACP client) while a tool call is in flight can permanently brick the session: every subsequent user prompt is answered only withQueued for the next turn. (N queued)and never runs. The only recovery is restarting thehermes acpprocess.Root cause
When a cancel lands mid tool call,
run_conversationreturns{"final_response": None, "interrupted": True, ...}— the key is present with valueNone, so inacp_adapter/server.py::prompt:The
AttributeErrorescapesprompt()before thestate.is_running = Falsereset at the end of the post-turn tail. The session is then stuck marked running forever:prompt failed ...: Internal error(acp.exceptions.RequestErrorviaacp/task/supervisor.py);if state.is_running:guard and is appended tostate.queued_prompts→Queued for the next turn. (N queued);prompt()call that can never start.Observed live (agent log excerpt):
Trigger condition: the cancel must land while the turn has produced no partial response text yet (e.g. during a long-running
terminaltool call), sofinal_responsecomes backNonerather than""or partial text.Fix (two layers)
final_response = result.get("final_response") or ""—Nonecan no longer reach.startswith().try/finallyso thestate.is_running/state.current_prompt_textreset is guaranteed. Any future exception in that tail can no longer leave the session stuck busy — it propagates to the client as a failed request, but the next prompt runs normally.The diff is larger than the logic change because the tail block is re-indented under the
try:;git diff -wshows the real delta.Tests
Two regression tests in
tests/acp/test_server.py:test_prompt_cancelled_turn_with_none_response_does_not_brick_session— cancelled turn returningfinal_response=Nonecompletes withstop_reason="cancelled", leavesis_running=Falseand an empty queue, and a follow-up prompt runs as a normal turn instead of being queued.test_prompt_tail_exception_still_releases_session— a forced exception inside the guarded tail (save_sessionraising) still resetsis_running/current_prompt_text.