Skip to content

fix(acp): clear stale interrupted prompt when a new turn starts - #56624

Open
golldyck wants to merge 1 commit into
NousResearch:mainfrom
golldyck:fix/acp-clear-stale-interrupted-prompt
Open

fix(acp): clear stale interrupted prompt when a new turn starts#56624
golldyck wants to merge 1 commit into
NousResearch:mainfrom
golldyck:fix/acp-clear-stale-interrupted-prompt

Conversation

@golldyck

@golldyck golldyck commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a state-machine bug in the ACP adapter. A /steer on an idle session can resurrect a prompt the user cancelled and moved on from.

state.interrupted_prompt_text powers the "Zed-interrupt salvage" feature, which replays an interrupted prompt when /steer arrives right after a client cancel (PR #18258). The problem is how the field is managed. It is set only in cancel(), when a turn is actively running. It is cleared only inside the /steer salvage block. No other path clears it, so this sequence leaks a stale value:

  1. Session is running prompt P1 = "refactor the auth module". User hits ESC, so cancel() stores interrupted_prompt_text = "refactor the auth module". Turn ends.
  2. User sends an ordinary prompt P2 = "what's the weather". It runs to completion. interrupted_prompt_text is still "refactor the auth module", because the normal turn never cleared it.
  3. User later types /steer be concise on the idle session. The salvage block sees the stale value as truthy and runs:
    "refactor the auth module\n\nUser correction/guidance after interrupt: be concise" as a real LLM turn.

The user expected to nudge the current (nonexistent) turn. Instead the agent re-executes an abandoned task from two turns ago.

Fix: clear interrupted_prompt_text at the turn-start transition, where is_running flips to True, so starting any real turn drops the stale salvage buffer. The /steer salvage path already consumes and clears the field before this point, so legitimate immediate-salvage (steer directly after a client cancel, with no intervening turn) is unaffected.

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • acp_adapter/server.py: in prompt(), set state.interrupted_prompt_text = "" in the is_running = True turn-start transition.
  • tests/acp_adapter/test_acp_commands.py: test_acp_normal_turn_clears_stale_interrupted_prompt checks that a running-cancel value is cleared by a completed normal turn, and that a subsequent /steer runs only the steer text.

How to Test

  1. scripts/run_tests.sh tests/acp_adapter/test_acp_commands.py -q. 7 tests pass, including the existing salvage test test_acp_steer_after_zed_interrupt_replays_interrupted_prompt_with_guidance.
  2. To prove the fix is load-bearing: revert the acp_adapter/server.py hunk and rerun -k clears_stale. The new test fails, since interrupted_prompt_text still holds the abandoned prompt and /steer resurrects it. Restore the hunk and it passes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(acp):)
  • I searched for existing PRs to make sure this isn't a duplicate (searched interrupted_prompt_text, none; open PR fix(acp): ignore idle cancel before next prompt #50461 touches idle-cancel cancel_event poisoning, a different field and path, and leaves this bug)
  • My PR contains only changes related to this fix
  • I've run the test suite and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • Documentation (README, docs/, docstrings): N/A (inline comment explains the clear)
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md / AGENTS.md: N/A
  • Cross-platform impact: N/A (scripts/check-windows-footguns.py clean)
  • Tool descriptions/schemas: N/A

interrupted_prompt_text is set only on a running-cancel (for the /steer
salvage feature) and cleared only inside the /steer salvage path. If the
user cancels a running turn, then sends an ordinary prompt that runs to
completion, the interrupted prompt is never cleared. A later /steer on the
now-idle session then sees the stale value as truthy and resurrects it,
silently re-running a task the user cancelled two turns ago and moved on
from.

Clear interrupted_prompt_text at the turn-start transition so any real
turn drops the stale salvage buffer. The /steer salvage path already
consumes and clears it before reaching this point, so legitimate
immediate-salvage (steer directly after a client cancel) is unaffected.

Adds a regression test; existing salvage/idle tests still pass.
@alt-glitch alt-glitch added type/bug Something isn't working comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists labels Jul 1, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused ACP regression fix. Current main still stores the cancelled prompt in acp_adapter/server.py:1220, then replays any retained value from idle /steer handling at acp_adapter/server.py:1341; the ordinary turn-start transition at acp_adapter/server.py:1384 does not otherwise clear it. The proposed clear occurs after the legitimate immediate-salvage branch has consumed its value, so it preserves that behavior.

The added regression test covers the stale-buffer sequence, and the affected files are unchanged from the PR base on current main, so the salvage should be clean.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@israellot

Copy link
Copy Markdown
Contributor

We hit this in production last week, so I went to adopt this patch and found it doesn't fully close the hole. Sharing the repro and what we ended up shipping, in case it's useful.

The incident. A turn was cancelled, arming interrupted_prompt_text. Four hours later an unrelated one-line request from a different user on the same session ("give me editor access to both of these sheets") was merged behind the abandoned prompt. The agent read the stale request as the directive, treated the live one as an afterthought, and restarted building spreadsheets nobody had asked for.

The turn-start clear runs too late

The clear lands at the is_running = True transition, but the plain-text "stop and send" salvage branch (server.py ~L1696-1704 on main) consumes the buffer earlier in the same prompt() call. So the buffer still hijacks the very turn that was supposed to drop it — this only protects the turn after the offending one.

This is verifiable with the test in this PR. On a clean main worktree with just this PR's server.py hunk applied:

>       assert fake.runs == ["what's the weather"]
E       assert ["refactor th... the weather"] == ["what's the weather"]
E         At index 0 diff:
E         "refactor the auth module\n\nUser correction/guidance after interrupt: what's the weather"
E         != "what's the weather"

The assert state.interrupted_prompt_text == "" on the line above passes; it's the very next assertion that goes red. (Separately, this PR's diff anchors on test_acp_steer_after_zed_interrupt_replays_interrupted_prompt_with_guidance, which is no longer on main — it'll need a rebase regardless.)

What we shipped

The clear is necessary but not sufficient. Three things together, each independently reverted and confirmed to turn a test red:

  1. Bound the buffer's lifetime. The salvage feature exists for clients implementing "stop and send" as cancel-then-submit — two protocol calls milliseconds apart. Nothing bounded it, which is the only reason a 4-hour-old prompt was still live. We stamp interrupted_prompt_at on arm and refuse the buffer outside a 30s window, clearing it either way so a stale one can't linger. An unstamped buffer fails closed.

  2. Don't arm from a turn that already answered. cancel() arms whenever is_running is true, but the final response is delivered while is_running is still true — it's only reset in the finally. A cancel landing in that post-turn tail (history persistence, provenance updates, queued drain) captures a request that was already fulfilled and delivered. We track response_delivered and refuse to arm past that point. This was the direct cause of our incident.

  3. Flip the merge precedence. Even when salvage is legitimate, f"{interrupted_prompt}\n\nUser correction/guidance after interrupt: {user_text}" puts the abandoned task first as a bare imperative and demotes the live message. Models read the leading imperative as the task — correctly, given that ordering. We put the new message first and append the interrupted one as explicitly labelled reference-only context, which still lets deictic follow-ups ("not that file") resolve.

We also routed both salvage call sites through shared consume/merge helpers so the /steer and plain-text paths can't drift apart — they had already diverged slightly.

One scoping note

This doesn't apply to main as it stands, but flagging it since it bit us: our fork delivers synthetic notifications through prompt(), and the unscoped clear wipes the buffer on those too. A background timer firing isn't the user moving on, so it shouldn't invalidate a pending correction. We gate the clear on not synthetic_notification. If anything upstream ever drives prompt() from a non-user event, the same scoping will be needed.

Happy to open a PR with the window + response_delivered + precedence changes on top of this one if you'd like — or feel free to fold any of it in here. Either way, thanks for spotting the underlying leak; the diagnosis in your description is right, it just needs the consumption-side gate to actually stop the replay.

israellot added a commit to YallaPlay/hermes-agent that referenced this pull request Aug 13, 2026
A cancelled prompt could be replayed as the leading instruction of an
unrelated turn hours later. Live incident 2026-08-12: a turn was
cancelled after it had already delivered its answer, which armed
state.interrupted_prompt_text; four hours on, a different user's
one-line request ("give me editor access to both of these sheets") was
merged BEHIND that abandoned prompt. The agent read the stale request as
the directive and restarted building spreadsheets nobody had asked for.

Three independent defects, each sufficient to cause the derail:

1. The salvage buffer had no lifetime. It is meant for clients that
   implement "stop and send" as cancel-then-submit, two protocol calls
   milliseconds apart. Nothing bounded it, so it survived indefinitely.
   Stamp it on arm and refuse it outside a 30s window, clearing the dead
   buffer so it cannot ambush a later prompt. An unstamped buffer fails
   closed (not salvageable).

2. cancel() armed the buffer from any running turn, including one that
   had already produced and delivered its final response. The response
   is delivered inside the post-turn tail while is_running is still True
   (server.py), so a late cancel captured an already-fulfilled request.
   Track response_delivered and refuse to arm once the turn has answered.

3. The merge put the interrupted prompt FIRST and demoted the user's
   live message to "User correction/guidance after interrupt: ...".
   Models correctly read the leading imperative as the task. Flip it:
   the new message leads, the interrupted one follows as explicitly
   labelled reference-only context so deictic follow-ups ("not that
   file") still resolve.

Both salvage call sites now share _consume_interrupted_prompt (gating)
and _merge_interrupted_prompt (wording), so the two paths cannot drift.

Also adopts the turn-start clear from upstream PR
NousResearch#56624. That fix alone is insufficient here:
it clears the buffer at the is_running=True transition, which runs
AFTER the plain-text salvage branch has already consumed it, so it only
protects the turn after the offending one. Verified by test — see the
upstream PR comment accompanying this change.

Restores coverage for the salvage paths, which our fork's test-prune
(3997561) had dropped entirely.
israellot added a commit to YallaPlay/hermes-agent that referenced this pull request Aug 13, 2026
The turn-start clear adopted from upstream PR NousResearch#56624 broke an existing
invariant: tests/acp/test_server.py::
test_notification_does_not_resurrect_cancelled_prompt asserts that a
SYNTHETIC notification delivery must leave a cancelled prompt intact for
the next real user prompt. Notifications route through prompt(), so the
unscoped clear wiped the buffer from a background event — the user never
moved on, a timer did. Gate the clear on `not synthetic_notification`.

This is a latent regression in the upstream PR as written, not just in
our adaptation; reported on the PR.

Also pins the clear with a test. Previously nothing failed when it was
removed, because on text-only turns the salvage branches consume the
buffer before it is reached. The case it genuinely covers is a turn that
SKIPS those branches — a multimodal prompt — which would otherwise leave
a live buffer for the next text prompt to pick up.

Full mutation check now red for all four guards: salvage window,
precedence order, response_delivered gate, and this turn-start clear.
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-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.

4 participants