Skip to content

fix(acp): ignore idle cancel before next prompt - #50461

Open
MauroFigueira wants to merge 2 commits into
NousResearch:mainfrom
MauroFigueira:fix/acp-ignore-idle-cancel
Open

fix(acp): ignore idle cancel before next prompt#50461
MauroFigueira wants to merge 2 commits into
NousResearch:mainfrom
MauroFigueira:fix/acp-ignore-idle-cancel

Conversation

@MauroFigueira

Copy link
Copy Markdown

Summary

  • Treat ACP cancel requests received while a session is already idle as no-ops.
  • Clear any stale cancel/interrupt state in that idle path so the next prompt is not immediately reported as interrupted.
  • Add regression coverage for both idle-cancel and true running-cancel behavior.

Root cause

Some ACP clients, observed with Xcode, can send a best-effort cancel immediately before submitting the next prompt. Hermes previously always set the session cancel event and called agent.interrupt() whenever a cancel request arrived, even if state.is_running was already false.

That stale interrupt then poisoned the next session/prompt: run_conversation() saw the pending interrupt before calling the model and returned interrupted_by_user. In Xcode this surfaced as:

Error: JSON-RPC Internal Error (code -32603)
The server encountered an internal error processing the request.

Fix

Only propagate cancel to the agent when the ACP session is actively running. If the session is idle, clear the cancel event and any stale agent interrupt instead.

This preserves real cancellation behavior for in-flight prompts while avoiding a stale idle cancel affecting the next user prompt.

Test plan

uv run --extra acp --extra dev python -m pytest tests/acp_adapter/test_acp_commands.py -q -o 'addopts='
# 8 passed

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the stale ACP interrupt path. The current-main premise is confirmed: acp_adapter/server.py:1221-1224 sets the cancel event and calls agent.interrupt() even while idle, while agent/conversation_loop.py:648-653 exits a new turn when that flag remains set.

Problems

  • tests/acp/test_server.py:342-347 currently creates an idle session, invokes cancel, and asserts that cancel_event is set. This PR intentionally clears that event for idle sessions, so the existing test will fail after applying the change.

Suggested changes

  • Update test_cancel_sets_event to model a running session (state.is_running = True) before asserting the real-cancel behavior. The new idle-cancel regression test can then cover the opposite contract.

Automated hermes-sweeper review.

Comment thread acp_adapter/server.py
# submitting the next prompt, even when the previous turn is
# already idle. Treat that as a no-op; otherwise a stale
# AIAgent interrupt poisons the next prompt and it returns
# interrupted_by_user without ever calling the model.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This intended idle no-op makes tests/acp/test_server.py:342-347 fail because that existing test cancels an idle session and asserts cancel_event.is_set(). Update that test to set state.is_running = True before asserting real cancellation behavior.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 517b579. The existing test now sets state.is_running = True (and is renamed to test_running_cancel_sets_event) before asserting the real-cancel behavior. The idle-cancel regression remains covered separately. Verified with both relevant test files: 88 passed.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@MauroFigueira

Copy link
Copy Markdown
Author

Addressed the sweeper feedback in 517b579: test_cancel_sets_event now explicitly models an active turn with state.is_running = True (and was renamed accordingly), while the idle-cancel regression test continues to cover the opposite contract.

Verification: 88 passed across tests/acp/test_server.py and tests/acp_adapter/test_acp_commands.py on the PR branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants