Skip to content

emrg: Esc stops the session's turn, not only this client's send - #2636

Merged
argszero merged 1 commit into
masterfrom
fix/esc-stops-any-turn
Oct 29, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/esc-stops-any-turn

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2635

The defect

The TUI's Esc-interrupt was gated on busy alone:

if data == b"\x1b" and busy:

busy is this client's own send in flight. A turn started by a scheduled task, or by another client, sets the other fact — turn_running, fed by the daemon's turn_start / done frames and by the resume snapshot, and already what the elapsed timer runs on — so with such a session open the timer counted, the turn was on screen, and Esc did nothing. Stopping it meant opening the GUI or killing the daemon. The daemon's cancel is session-scoped, so stopping the same turn from any watching client is legal; only the client's own question was too narrow.

The change

One named predicate, esc_stops_the_session_turn(busy=…, turn_running=…), and the branch asks it:

if data == b"\x1b" and esc_stops_the_session_turn(busy=busy, turn_running=turn_running):

busy is not replaced by the wider fact — a turn this client just sent is stoppable in the window before the daemon's turn_start arrives. The keystroke still only asks: request_cancel sends the frame and the daemon's cancelled receipt remains the only statement that anything stopped. The GUI is unchanged; it already reads the session-scoped busyBySid[activeSid].

Tests

  • tests/test_app_cancel_session.py — four rows: a turn this client did not start is stoppable; a send in flight still is; neither fact means no request; and a wiring row asserting the call site hands the predicate both variables rather than a constant (the shape that survived here once already, cf. test_the_receipt_asks_the_two_fact_question). The existing Esc-branch anchor moved to the new judgement with every assertion kept.
  • emrg/gui/renderer/src/components/Shell.test.tsx — the GUI's existing behaviour pinned: a turn_start broadcast the client never sent for shows the stop button, and clicking it cancels with that session id.

Mutations, each printed before it ran and each reverted

Mutation Result
return busy or turn_running → return busy test_esc_stops_a_turn_this_client_did_not_start red, 1 failed, 24 passed
the cancel frame loses session_id test_the_cancel_names_the_session_and_nothing_else and test_the_bare_connection_scoped_cancel_is_gone red, 2 failed, 23 passed
Shell's busy = appState.busyBySid[activeSid] → busy = false the new Shell test red, 1 failed, 49 passed

The first attempt at the first arm reverted with git checkout -- emrg/client/app.py while the change was still uncommitted, which discarded the production edit instead of the mutation. It was re-applied and committed before the arms were re-run, so each revert above restores the commit.

Verification

  • tests/test_app_widgets.py::test_the_tool_selector_closes_on_esc_before_the_turn_cancel_can_see_it anchors on the Esc heading, which this renames; its anchor moved and its order property is untouched.
  • Adding one renderer test moves two documented counts, so Agent.md moves with them: renderer 739 → 740, Shell 49 → 50, both char-neutral and under the prompt cap.
  • Full Python suite: 5992 passed, 36 skipped in 274.58 s. Renderer: npm run typecheck clean, npm test 740 passed.

CI: pending at push time — nothing here is claimed as green.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261029-144404

Landed-tree readings taken before this vote, against base b079a2bd (master's own tip): check-merge-freshness.py 2636 answers FRESH — master is an ancestor, and the head has a passing run — and the landing change is 5 files, +110/−7.

The diagnosis is right and the fix is the bounded way to make it. The Esc branch asked busy alone, which is this client's own send in flight, so a turn a scheduled task or another client started was on screen with the elapsed timer counting while the keystroke did nothing. The new predicate asks both facts and neither replaces the other: busy still covers the window between a send and the daemon's turn_start frame, which the wider fact cannot see yet, and turn_running is the session-scoped fact the timer was already reading. The keystroke still only asks — request_cancel sends the frame and the daemon's cancelled receipt remains the only statement that anything stopped, so no client narrates a stop the session never took.

What makes this reviewable rather than merely plausible is the wiring row. test_the_esc_branch_asks_both_facts reddens if the call site hands the predicate a constant, which is exactly the defect that would leave every predicate test green and restore the reported behaviour. The GUI half is a pinning test and not a behaviour change — busyBySid[activeSid] was already session-scoped — and it asserts the daemon bridge is reached for a turn this client never sent.

Both CI legs passed on a826181e (run 40830216587).

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261029-144940

Landing tree 7077c236a0c1a5a5355db7ebf7ae5e10475264ca — the head's own tree: check-merge-freshness.py reads FRESH (ahead of master, behind by 0, merge base b079a2bd is master's tip), so the green CI on a826181e is a reading about exactly the tree a merge would land. Full suite on that tree is CI's two legs, both green.

What I measured here, on that tree rather than on the description:

  • The predicate, driven both ways. esc_stops_the_session_turn(busy=False, turn_running=True) is True; (busy=True, turn_running=False) is True; (False, False) is False. Driven directly on purpose: both modules these tests live in are whole-module win32 skips on this host — pytest prints 109 skipped and nothing else — so a green local run there would have been a reading of nothing at all.
  • Both mutation arms are caught, each in a fresh process (module caching has made an arm lie here before). Reverting return busy or turn_running to return busy reddens a turn this client did not start is not stoppable. Handing the call site a constant (turn_running=False) leaves the predicate rows green and reddens only the wiring row — which is the defect that row exists for. The first version of that second arm replaced the whole if line instead of the argument and died with IndentationError; that is an arm that never landed, so it was rewritten and re-run rather than counted as a catch.
  • The branch still only asks. request_cancel( is inside the block, nothing is narrated there, and the old narrow and busy: branch is gone from the tree.

The renderer row pins a path that already exists end to end rather than a mock of something absent: preload.js:83 exposes cancel, main.js handles emrg:cancel by naming the session, and Shell.tsx:449 already reads busyBySid[activeSid]. The Agent.md count change is covered by CI's own check-node-test-count.py step, which asks the runners what they executed and fails if the document disagrees — I have no node_modules in a fresh worktree, so that one claim is CI's reading and I am stating it as such.

Nothing here asks for a change: one named predicate, the branch asking it, and the tests that hold both facts.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261029-150148

Measured on this head's own tree, not on the PR page: a detached worktree at a826181e whose emrg.__file__ resolves inside that worktree, with the main checkout on master at b079a2bd.

Baseline. The five Esc rows in tests/test_app_cancel_session.py plus test_app_widgets.py::test_the_tool_selector_closes_on_esc_before_the_turn_cancel_can_see_it pass by node id; both touched files run whole at 107 passed, 2 skipped.

The predicate, both ways. return busy or turn_running → return busy reddens test_esc_stops_a_turn_this_client_did_not_start at line 348, and the other two predicate rows stay green — so the wider fact is what carries the reported case, and the row that names it is the row that catches its removal.

The call site, both ways. This is the arm that matters, because the predicate's own rows pass against a call site that hands it anything. busy=True at the call site reddens test_the_esc_branch_asks_both_facts at line 377; turn_running=False — which restores exactly the behaviour that was reported — reddens the same row while test_esc_stops_a_turn_this_client_did_not_start stays green. That is the pair that makes the wiring row load-bearing rather than decorative: without it, the defect this PR fixes could be reinstated with every other row still passing. Each arm printed its mutated line, and each was restored from saved bytes, git status --porcelain empty afterwards and the head re-read as a826181e.

The keystroke still only asks. The Esc branch reaches request_cancel only inside the predicate guard, request_cancel sends nothing when there is no session to name, and cancelled_line remains the sole place a stop is narrated — test_the_esc_branch_asks_and_shows_nothing holds that as an absence. So this change cannot make a stray Esc claim a stop the daemon did not report; test_request_cancel_sends_the_session is the row that pins the frame itself, carrying the session id.

Freshness: check-merge-freshness.py 2636 answers FRESH — the merge base is master's tip, and the head has a passing run. Both legs passed on a826181e (run 40830216587).

@argszero

Copy link
Copy Markdown
Owner Author

Correction to the approval above: the row that pins the cancel frame is test_the_cancel_names_the_session_and_nothing_else — it asserts the whole frame {"type": "cancel", "session_id": …} — not test_request_cancel_sends_the_session, which is a name I wrote from memory and which does not exist in this tree. The claim that row supports stands unchanged; only the citation was wrong, and a review that cites a test by a name no file carries is not a review of that test.

The other direction is pinned too: test_a_cancel_with_no_session_is_not_sent asserts no frame at all when there is no session to name. — cycle cyc20261029-150148

@argszero
argszero merged commit bb36bff into master Oct 29, 2026
2 checks passed
@argszero argszero mentioned this pull request Oct 29, 2026
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.

TUI: Esc does not stop a turn the client did not start (scheduled task or another client)

2 participants