Skip to content

fix: Esc reliably stops the turn, not just the round - #325

Merged
martian56 merged 2 commits into
mainfrom
claude/esc-button-agent-stop-r85jex
Jul 28, 2026
Merged

fix: Esc reliably stops the turn, not just the round#325
martian56 merged 2 commits into
mainfrom
claude/esc-button-agent-stop-r85jex

Conversation

@martian56

@martian56 martian56 commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Esc set the interrupt flag and canceled the stream token, but the flag was only consulted at coarse boundaries, so a turn could keep working after the user asked it to stop. Canceling the token reliably kills an in-flight model stream; everything else depended on the flag.

Three gaps, which together explain why Esc felt intermittent rather than simply broken.

1. A race in the approval and question handlers

_approval_key answered the prompt with decide(0) and only then called request_interrupt(). decide() releases the blocked turn goroutine immediately, so which behavior you got depended on which goroutine won the scheduler: the turn could return from the approval, run the next tool call, and start another round before the flag was ever set. _question_key had the same ordering. The same keypress produced different results run to run.

Both handlers now raise the interrupt first and answer second, which makes the stop deterministic.

2. No interrupt check between the tool calls of one batch

run_turn checked the flag at the top of each round and once before the batch, never inside it. A batch of five calls that got an Esc during the first still ran the other four: read-only ones silently, mutating ones by putting another approval prompt in front of a user who had already asked to stop. run_subagent had the same gap.

Both now check before each call. Calls that never ran still get a tool result recorded so the conversation stays balanced for a resume, and the fan-out path waits only for the agents it actually started rather than for slots that will never be filled.

3. Queued approvals re-prompting after the stop

Askers serialize on ask_lock, so with parallel subagents each waiting caller published its own prompt once the previous one cleared, and the user had to dismiss one prompt per agent before the turn ended. ask_detail and ask_question now answer deny/decline without prompting when the interrupt is already set.

Tests

  • An e2e turn where Esc at the first of two writes in one batch leaves neither file written, never offers the second for approval, and still balances the history.
  • Unit tests for both ask guards.
  • A subagent test covering the same guard through the subagent path.

CI is green on both the Linux and Windows matrix jobs, covering build, tests, behavior evals, fmt --check, and docs.

Not covered

web_fetch, web_search, and MCP tool calls ignore the cancel token — dispatch_cancelable only threads it into run_command. An Esc during one of those still waits out its timeout. Fixing that needs cancelable transports rather than a flag check, so it is left as separate work.

One related behavior is deliberately unchanged: a message typed and queued during a turn still sends after Esc ends that turn, because the next tick picks it up from the queue. Discarding the user's own queued text on Esc is a product decision rather than a bug fix, so it is not part of this PR.

Esc set the interrupt flag and canceled the stream token, but the flag was
only consulted at coarse boundaries, so a turn could keep working after the
user asked it to stop.

Three gaps, all of which made Esc look intermittent:

A race in the approval and question handlers. Both answered the prompt with
decide() and only then called request_interrupt(). decide() releases the
blocked turn goroutine immediately, so whether the stop landed depended on
which goroutine won: the turn could finish the call and move on before the
flag was ever set. Both handlers now raise the interrupt first and answer
second, which makes the stop deterministic.

No interrupt check between the tool calls of one batch. run_turn checked the
flag at the top of each round and once before the batch, never inside it. A
batch of five calls that got an Esc during the first still ran the other
four: read-only ones silently, mutating ones by putting another approval
prompt in front of a user who had already asked to stop. Both run_turn and
run_subagent now check before each call. Calls that never ran still get a
tool result recorded so the conversation stays balanced for a resume, and
the fan-out path waits only for the agents it actually started.

Queued approvals re-prompting after the stop. Askers serialize on ask_lock,
so with parallel subagents each waiting caller published its own prompt once
the previous one cleared, and the user had to dismiss one per agent before
the turn ended. ask_detail and ask_question now answer deny/decline without
prompting when the interrupt is already set.

Tests: an e2e turn where Esc at the first of two writes in a batch leaves
neither file written, never offers the second, and still balances the
history; unit tests for the two ask guards and the subagent path.

Not covered: web_fetch, web_search, and MCP tool calls ignore the cancel
token, so an Esc during one waits out its timeout. That needs cancelable
transports rather than a flag check.
@martian56 martian56 self-assigned this Jul 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
src/agent/turn_e2e_test.rv (1)

442-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for Esc landing mid-batch during a dispatch fan-out.

This test only covers the sequential (non-fanout) batch path. The trickiest new logic in turn.rv — waiting for already-started dispatch goroutines via fanout_results.count() < stopped before recording placeholders (turn.rv lines 196-206) — isn't exercised by any test here. Consider a variant combining serve_fanout-style dispatch calls with _interrupt_when_pending (interrupting while the first subagent's write approval is pending) to confirm the turn still ends cleanly and waits only for the started subagent.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/agent/turn_e2e_test.rv` around lines 442 - 494, Add a companion test for
Esc during fan-out dispatch, using serve_fanout-style calls and
_interrupt_when_pending while the first subagent’s write approval is pending.
Assert the turn ends cleanly, only the already-started subagent is awaited,
later dispatches are not started, and the resulting tool history remains
balanced with the expected interruption placeholder.
src/agent/turn.rv (1)

129-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate the two "batch was interrupted" branches.

Lines 134-144 duplicate the stopped == 0 case of the unified block at lines 196-216 (placeholder loop, Interrupted event, _report_usage, _end_turn). The only reason for the separate early check is to skip the "running N agents in parallel…" message — worth preserving, but the balanced-history bookkeeping shouldn't live in two places since it's easy for a future edit to update one copy and miss the other.

♻️ Suggested consolidation
         st.push_assistant_calls(reply_text, calls)
-        // Esc may have landed while the model was streaming this tool request.
-        // Skip running the batch so a tool the user pre-approved this turn does
-        // not execute after they asked to stop, but still record a result for
-        // each call so the conversation stays balanced for a resume.
-        if st.interrupted() {
-            let c = 0
-            while c < calls.len() {
-                st.push_tool_result(calls[c].id, calls[c].name, "(interrupted by user before running)")
-                c = c + 1
-            }
-            st.emit(DisplayEvent.Info("Interrupted"))
-            _report_usage(st, model)
-            _end_turn(st)
-            return
-        }
         let fanout = _count_dispatch(calls) >= 2
         let fanout_results = ToolResults.new(calls.len())
-        if fanout {
-            st.emit(DisplayEvent.Info("running ${_count_dispatch(calls)} agents in parallel…"))
-        }
         let i = 0
         // The index of the first call left unrun by an Esc mid-batch, -1 when
-        // the whole batch ran.
+        // the whole batch ran, 0 when Esc landed before any call started.
         let stopped = 0 - 1
-        while i < calls.len() {
-            if st.interrupted() {
-                stopped = i
-                break
+        // Esc may have landed while the model was streaming this tool request;
+        // skip running the batch (and the "running in parallel" announcement)
+        // entirely, but still record a result for each call so the
+        // conversation stays balanced for a resume.
+        if st.interrupted() {
+            stopped = 0
+        } else {
+            if fanout {
+                st.emit(DisplayEvent.Info("running ${_count_dispatch(calls)} agents in parallel…"))
             }
-            let call = calls[i]
-            ...
-            i = i + 1
+            while i < calls.len() {
+                if st.interrupted() {
+                    stopped = i
+                    break
+                }
+                let call = calls[i]
+                ...
+                i = i + 1
+            }
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/agent/turn.rv` around lines 129 - 216, Consolidate the pre-batch
interruption handling in the turn-processing flow with the later stopped-batch
branch, using the existing stopped state to represent zero calls started.
Preserve the early interruption behavior of suppressing the parallel-agent
status message, while keeping placeholder tool results, the Interrupted event,
usage reporting, and _end_turn in one shared path. Update the logic around
_count_dispatch, the stopped variable, and the final stopped >= 0 block without
changing normal batch execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/agent/turn_e2e_test.rv`:
- Around line 442-494: Add a companion test for Esc during fan-out dispatch,
using serve_fanout-style calls and _interrupt_when_pending while the first
subagent’s write approval is pending. Assert the turn ends cleanly, only the
already-started subagent is awaited, later dispatches are not started, and the
resulting tool history remains balanced with the expected interruption
placeholder.

In `@src/agent/turn.rv`:
- Around line 129-216: Consolidate the pre-batch interruption handling in the
turn-processing flow with the later stopped-batch branch, using the existing
stopped state to represent zero calls started. Preserve the early interruption
behavior of suppressing the parallel-agent status message, while keeping
placeholder tool results, the Interrupted event, usage reporting, and _end_turn
in one shared path. Update the logic around _count_dispatch, the stopped
variable, and the final stopped >= 0 block without changing normal batch
execution.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2104ee02-e87f-4c5f-97ed-888f008df3d6

📥 Commits

Reviewing files that changed from the base of the PR and between 66a6430 and 424d33c.

📒 Files selected for processing (7)
  • src/agent/state.rv
  • src/agent/state_test.rv
  • src/agent/subturn.rv
  • src/agent/subturn_test.rv
  • src/agent/turn.rv
  • src/agent/turn_e2e_test.rv
  • src/app/update.rv

Addresses both review nitpicks on the Esc fix.

The pre-batch interrupt check duplicated the placeholder/Interrupted/usage/
end-turn bookkeeping that the mid-batch path already does, so a later edit
could easily update one copy and miss the other. Removed it: the check at the
top of the call loop produces exactly the same outcome at i=0, which makes
"Esc beat the batch" simply stopped=0 and leaves one place doing the
bookkeeping. The only thing the early return was still buying was suppressing
the "running N agents in parallel" line for a batch that will not run, so
that announcement is now guarded directly.

The fan-out branch of the stopped block was untested. The new e2e test uses a
dispatch, write_file, dispatch batch: the write's approval gives Esc a
deterministic place to land with the first agent already spawned and the
second dispatch still ahead. It asserts the started agent is awaited, the
unstarted one never runs, the denied write never happens, and all three calls
still carry a result in order. Reaching the end of the drain is itself the
assertion that the wait does not block on a slot nothing will fill.
Repository owner deleted a comment from coderabbitai Bot Jul 27, 2026
@martian56
martian56 merged commit 82d7b94 into main Jul 28, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants