Repository navigation
emrg: Esc stops the session's turn, not only this client's send - #2636
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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)isTrue;(busy=True, turn_running=False)isTrue;(False, False)isFalse. Driven directly on purpose: both modules these tests live in are whole-modulewin32skips on this host —pytestprints109 skippedand 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_runningtoreturn busyreddensa 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 wholeifline instead of the argument and died withIndentationError; 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 narrowand 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
left a comment
There was a problem hiding this comment.
✅ 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).
|
Correction to the approval above: the row that pins the cancel frame is The other direction is pinned too: |
Closes #2635
The defect
The TUI's Esc-interrupt was gated on
busyalone:busyis 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'sturn_start/doneframes 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:busyis not replaced by the wider fact — a turn this client just sent is stoppable in the window before the daemon'sturn_startarrives. The keystroke still only asks:request_cancelsends the frame and the daemon'scancelledreceipt remains the only statement that anything stopped. The GUI is unchanged; it already reads the session-scopedbusyBySid[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: aturn_startbroadcast 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
return busy or turn_running→return busytest_esc_stops_a_turn_this_client_did_not_startred,1 failed, 24 passedsession_idtest_the_cancel_names_the_session_and_nothing_elseandtest_the_bare_connection_scoped_cancel_is_gonered,2 failed, 23 passedbusy = appState.busyBySid[activeSid]→busy = false1 failed, 49 passedThe first attempt at the first arm reverted with
git checkout -- emrg/client/app.pywhile 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_itanchors on the Esc heading, which this renames; its anchor moved and its order property is untouched.Agent.mdmoves with them: renderer739 → 740, Shell49 → 50, both char-neutral and under the prompt cap.npm run typecheckclean,npm test740 passed.CI: pending at push time — nothing here is claimed as green.