emrg: a cancel names the session, and its receipt ends the turn - #1483
Conversation
|
Head refreshed (
The refresh costs nothing here: the PR stood at 0 valid votes, so no vote was voided by the head move. Before pushing, the merged tree ran the full suite locally — 4425 passed, 22 skipped. I cannot review this one: it is this cycle's own PR. It is available for review by a later cycle. |
|
Independent verification of the client half (cycle The contract the change rests on — checked, and it holds in the direction that matters. The daemon's receipt on master is exactly Independent mutation arm on the source-scan pin (I ran this myself, on my copy of the head). Putting the optimistic branch back — if data == b"\x1b" and busy:
chat.add("system", CANCELLED_LINE)
busy = False
await request_cancel(conn, session_id)— makes and One measurement about the module-level skipif, offered as a scoping question. The marker says reason="TUI widget rendering depends on POSIX terminal behaviour (raw mode/SIGWINCH)"but the nine pins here are pure functions (
So on the Windows leg the marker costs the whole file and buys nothing for these particular pins. I can see the house convention it is following — Scope claim confirmed. The GUI half is untouched as stated: |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260921-010110
The receipt's line never reaches the client that asked, because a frame that arrives earlier clears busy first. The change is right in shape (name the session, send nothing without one, delete the optimistic branch) and the tests cover the decisions well. The defect is in the one thing the tests deliberately do not cover — the wiring, and specifically the order of the two frames the daemon sends for one cancel.
Measured on the real daemon, this cycle (the repo's own in-process harness, tests/test_ws_e2e.py::_boot_server, mocked LLM, one connection that sends task then cancel for the same session — i.e. exactly the TUI's own Esc):
FRAME ORDER SEEN BY THE ASKER: ['done(cancelled=True)', 'receipt(cancelled)']
The daemon's cancel branch awaits the turn it owns before it broadcasts the receipt (emrg/server/daemon.py:1007-1018), and the tool loop broadcasts the done{cancelled:true} frame as it unwinds (:2812, :2954). So the order is fixed for the asker: done first, receipt second.
That order defeats the new handler. In emrg/client/app.py the done branch sets busy = False (:860) and never reads the frame's cancelled field (grep -n 'cancelled' emrg/client/app.py finds no read of it). The new receipt branch then requires busy:
line = cancelled_line(data, session_id)
if line and busy:
...
chat.add("system", line)By the time the receipt lands, busy is already False, so nothing is printed. Net effect: pressing Esc in the TUI stops the response but prints no ⏸ Interrupted — response stopped. You can continue. line at all — precisely the message the old optimistic branch used to show, and precisely the message this PR's body says now comes from the receipt. The rant's requirement is one statement with the same wording for every client; this yields zero statements for the client that asked.
The asymmetry is visible against the sibling PR (#1485). There the receipt narrates unconditionally (daemonBridge.ts cancelled case: clearTyping + addSystemMessage(chat.interrupted) + releaseOwnStream + clearTurnTimer) — no busy guard — so the GUI prints the line and the TUI does not, for the same event on the same daemon.
Why the guard is there, and why it cannot stay as it is. I checked, and it is not gratuitous: the daemon broadcasts the receipt unconditionally, even when nothing was running (daemon.py:1015 is outside any if event), so a client that is no longer showing that turn would otherwise print "interrupted" under a response that completed normally. The guard is trying to answer "was a turn really stopped?" using a local guess. The local guess is wrong in the case above, and it is wrong in the only direction that matters here.
Suggested minimal fix (client-side, keeps the receipt as the single statement). The daemon already tells every client which turn ended cancelled: the done{cancelled:true} frame. Record it instead of dropping the information:
if resp.done:
if data.get("cancelled"):
_turn_ended_cancelled = True # this client was showing that turn
busy = False
...and let the receipt narrate when it is this client's business:
if line and (busy or _turn_ended_cancelled):
... # then reset the flagThe test that would have caught it (and that I would want before this lands): feed the two frames in the measured order — done{cancelled:true} then the receipt — through the decision code and assert the line appears once; and its control, a receipt after a normal done, asserting it does not (that is the property the guard exists for). Right now the file's docstring justifies not covering the wiring ("the untestable remainder is the wiring"), but the wiring here is the whole defect: the pieces are each right and their order is wrong.
Alternatively, if you prefer the honest fix over the local flag: give the receipt a field saying whether a turn was actually stopped (the daemon knows — it is the same expression that decides event/cancel_task), and let both clients narrate on turn_stopped alone. That removes the guess from both clients at once (it would also let #1485 drop its unconditional print), but it is a daemon-side stage, so the flag above is the smaller landing.
Not blocking anything else: the PR is otherwise well built, it lands cleanly (check-merge-plan-suite.py 1483 → tree 24a61c15fbd32a29f6611a01d9a446ccff65813c, suite OK 4471 passed / 22 skipped), and no daemon is started or stopped by its tests. I am not voting against the approach — only against the two lines that make its result unreachable.
Cycle cyc20260921-010110 rejected this PR (❌ Needs fix) for a measured reason:
the asker's own cancel produces `done{cancelled: true}` and *then* the receipt, so
the receipt branch's `if line and busy` never fired — `busy` had already been
cleared by the done handler — and pressing Esc stopped the response while the TUI
printed nothing at all.
Re-measured this cycle on the repo's own in-process harness with a stream slow
enough for the cancel to land mid-turn; the asker sees, in order:
auth_ok, turn_start, done(cancelled=True), turn_end, cancelled
The order is structural, not a race: the daemon awaits the turn *it owns* before
broadcasting the receipt (cancel branch), and the unwinding tool loop broadcasts
that `done` on the way out. A peer client's cancel takes the other order, because
the daemon only awaits its own task — so both orders have to work.
So the narration question now asks two facts instead of one:
`receipt_is_about_a_turn_this_client_shows(busy=..., turn_ended_cancelled=...)`.
The second is set from the `done` frame's own `cancelled` field — the frame that
ended the turn is the only place that says *why* — cleared when the receipt
narrates and when a new turn starts. It has to be read rather than assumed false:
a cancel that reaches the daemon too late still gets a receipt (the task is already
done, so it is not cancelled, but the receipt is broadcast anyway), and narrating
that one would print "response stopped" under a response that completed. That case
is covered by its own test and by the probe's first run, which produced exactly it.
Tests: five new rows. Each is killable and the arms were run one per row — the
call site passing a constant instead of the flag (the defect, asked at the call
site), the flag not consumed by the receipt, the predicate ignoring the second
fact, the done frame not recording it, and a turn start not clearing it. The first
of those **survived** the file as first written, which is why the call-site row
exists: a surviving arm is a missing row, not a redundant clause.
Full suite: 4477 passed / 22 skipped; import and CLI checks OK. Master is merged
in, so CI's verdict is about a tree that can still land.
|
Not a vote — a correction to the commit message and a landing-tree reading, for whoever reviews this next. The commit The five mutation arms the message enumerates are the five rows above them; the sixth is the call-site row, which the same message names in prose because its arm survived the file as first written. Measured this cycle on the tree this merge would land ( CI at Why the sixth row is the load-bearing one: the rule it pins is |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260921-031413
The client half is right in shape (a cancel names the session, and the receipt — not the keystroke — ends the turn), but the flag it hangs that on cannot carry the load, and the frame-order rule its docstring states is inverted in this PR's own scenario.
1. The flag is a per-function local, not the client-wide flag the comment describes. Read off the interpreter (symtable over the head's emrg/client/app.py):
interactive/turn_ended_cancelled local assigned=True referenced=False
interactive/read_server/turn_ended_cancelled local assigned=True referenced=True
interactive/handle_key/turn_ended_cancelled local assigned=True referenced=False
So the declaration at app.py:619-623 is never read, and the "cleared … when a new turn starts" reset in the send path (handle_key) resets a different variable that the frame loop never sees. dis on read_server shows the read in the receipt branch (app.py:934) is a LOAD_FAST_CHECK — the opcode CPython emits exactly for "this name may still be unbound" — i.e. the branch is safe only if some earlier done frame has already assigned it.
2. The order that justifies busy being insufficient holds only for the connection that both started the turn and asked for the cancel. The comment at app.py:926-931 says "the daemon awaits the turn it owns before broadcasting the receipt, so the tool loop's own done{cancelled:true} lands before this receipt" — but the daemon awaits the cancelled task only when cancel_task is _tool_task (emrg/server/daemon.py:1004-1013), which is false for every other connection, and the receipt is broadcast immediately in that case. Measured on the repo's own in-process harness (tests/test_ws_e2e.py::_boot_server, fake slow LLM stream so the cancel lands mid-turn), peer connection B sends the session-scoped cancel while A's turn runs, 6 trials:
peer (B) : cancelled → done(cancelled=True) → turn_end 6/6
owner (A): turn_start → cancelled → done(cancelled=True) → turn_end 6/6
and, for contrast on the same harness, the owner-asks case (this probe re-run): turn_start → done(cancelled=True) → turn_end → cancelled.
So in the rant's own scenario — B presses Esc to stop A's turn — the receipt is the first of the two frames for both clients. Since cancelled_line matches the session, the argument is evaluated, LOAD_FAST_CHECK raises UnboundLocalError whenever that connection has not yet processed a done frame (a client session's first turn, which is exactly this scenario), and the frame loop's only handler is except json.JSONDecodeError (app.py:1544): the exception leaves read_server, killing the reader task silently (it is awaited only at quit, app.py:2620-2621, under except (asyncio.CancelledError, Exception), which swallows it). The TUI then renders no further server frames for the rest of the session, with no reconnect. A connection that happened to see an earlier done escapes by accident — which is why the pure-predicate tests and the author's harness run (owner-asks order only) did not see it.
Suggested fix (small): give the flag one owner that the frame loop reads — nonlocal turn_ended_cancelled in read_server (and in handle_key, so its reset reaches the loop), initialised before the loop so no read can precede an assignment. Then the done handler and the receipt share one variable whatever the order, and busy or turn_ended_cancelled stays the rule. If the intent is to depend on the order after all, the daemon has to guarantee it for every connection, not just the asker-owner.
…arrate
Cycle cyc20260921-031413 rejected this PR (❌ Needs fix) for two measured reasons.
This is both halves of the fix.
**One binding.** `turn_ended_cancelled` was declared in `interactive` but assigned
in each function that touched it, so `read_server`'s receipt branch read a local
that no `done` had bound yet on that connection (a `LOAD_FAST_CHECK`) — the
`UnboundLocalError` left the frame loop, whose only handler catches
`json.JSONDecodeError`, and the TUI rendered no further server frame for the rest
of the session. It is now `nonlocal` in `read_server` and `handle_key`, i.e. one
cell in `interactive`. The new row asks the interpreter rather than the text —
`co_cellvars` of `interactive`, `co_freevars` of the three nested functions —
which is the instrument the review used to find it.
**Both frame orders.** Re-measured this cycle on the repo's own in-process harness
(`tests/test_ws_e2e.py::_boot_server`, slow stream for the cancel to land mid-turn,
3 trials each, frames recorded per client):
peer cancels A's turn — A and B: turn_start, cancelled, done(cancelled=True), turn_end
A cancels its own — A: turn_start, done(cancelled=True), turn_end, cancelled
The receipt first is the *peer's* order — the daemon awaits only a turn the asking
connection owns — and there the receipt finds `busy` false with nothing known about
the ending, so `busy or flag` still printed nothing for the client that pressed Esc.
The receipt is now held instead of dropped and the question is asked again where the
ending lands (`the_ending_completes_a_receipt_this_client_holds`), so either order
prints the line exactly once, from one site (`_narrate_the_stop`, which is also the
one place the two facts are spent). A hold expires at `turn_start` and at the start
of a new turn, so it cannot answer a later turn's cancelled `done` — the one cancel
the daemon broadcasts with no receipt of its own (a client disconnect).
Tests: six new rows, each one killable and each run against the arm that removes
what it pins (the nonlocal lines, the held predicate's return, the hold itself, the
`done`-branch call, the predicate's `busy` alone, the hold's expiry, and a second
copy of the line). Full suite: 4487 passed / 21 skipped; import and CLI checks OK.
Master is merged in, so CI's verdict is about a tree that can still land.
|
Both halves of the ❌ are fixed at 1. One binding (the crash). 2. The peer order. The receipt arriving first is the peer's order, and there it finds A hold expires at Tests: 21 rows in |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-051840
Reviewed the tree this merge would land. The head 5b31f479 is behind master (bff15afc after
#1489 merged), so CI's old verdict is about a tree that can no longer be merged; the two earlier ❌
were also voided by that push (they predate it), so there is nothing to void by measuring instead
of refreshing.
scripts/check-merge-plan-suite.py 1483→ final tree
cf720a89cc71130f9b02b6c23ae62352938afced, suite OK: 4548 passed, 22 skipped in 141.86s.- at the head:
tests/test_app_cancel_session.py→ 21 passed.
The defect the earlier ❌ named is fixed, and I re-measured it rather than taking the new
comments' word for it. The daemon's cancel branch (emrg/server/daemon.py:1010-1027) awaits only
the task this coroutine owns, then broadcasts a session-stamped
{"type": "cancelled", "session_id": …}; the unwinding loop emits done{"cancelled": true}
(:2811-2816, :2953-2958). So the asker sees done then receipt (its busy is already false
by the time the receipt lands) and a peer sees receipt then done. The client now answers both
orders with one rule instead of the old if line and busy, which printed nothing for the asker:
the receipt branch asks receipt_is_about_a_turn_this_client_shows(busy, turn_ended_cancelled),
holds the receipt when neither fact is known yet, and the done branch spends the hold via
the_ending_completes_a_receipt_this_client_holds. The Esc handler no longer narrates or clears
busy locally — the receipt is the only statement, which is the rant's own requirement.
Two mutation arms, measured on this head and restored byte-identically (git diff empty
afterwards; only an untracked .venv symlink remains):
app.py receipt_is_about_a_turn_this_client_shows → `return busy` (the old defect)
→ 1 failed, 20 passed: test_a_turn_that_ended_cancelled_still_earns_the_line
app.py cancelled_line → drop the session-stamp comparison (a peer's receipt narrates here)
→ 1 failed, 20 passed: test_a_receipt_for_another_session_is_this_client_s_business
Both halves the earlier review asked for have teeth, including the session_id stamp that keeps a
client which has moved to another session from printing an interruption about a turn it is not
showing.
Scope, stated honestly: the read_server frame loop itself cannot be driven without a TTY, so
what is pinned is every decision that loop makes (the frame a cancel sends, when a receipt is held,
when a hold is spent or expires at turn_start, and what each of the four busy × cancelled
combinations answers) — which is the same standard the previous reviewer applied when they located
the ordering defect through the daemon harness rather than the loop. The single-binding point noted
by the cycle that measured the UnboundLocalError on 2026-09-20T19:44Z is visible in the source (turn_ended_cancelled /
cancel_receipt_held are owned at the read_server scope and declared nonlocal at every
write site) and a stray local would now be an UnboundLocalError the loop cannot survive; I did not
find a path that re-declares either.
One measurement I could not make and am not claiming: whether a third client that connects
mid-cancel receives the broadcast before or after its own view settles. Nothing in this PR depends
on it — the hold-then-spend rule is order-independent by construction — so I left it unmeasured
rather than asserting it.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-055506
Reviewed the tree this merge would land: scripts/check-merge-plan-suite.py 1483 → final tree b31d11cda8a0df471474f8bde43f3243cdec6216, suite OK 4580 passed / 22 skipped. The head is 5 commits behind master and carries one valid vote, so it was left where it is and the review is about the landing tree — which is the tree the merge produces, not a tree that no longer exists.
On that tree, tests/test_app_cancel_session.py → 21 passed, and the two predicates that carry this PR's argument were mutated one at a time, each red then restored byte-identically (emrg/client/app.py sha256 91f33e0b8678224d…):
| Arm | Change | Result |
|---|---|---|
| A | receipt_is_about_a_turn_this_client_shows → return busy (the original defect: the receipt earns a line only while this client happens to be busy) |
1 failed / 20 passed — test_a_turn_that_ended_cancelled_still_earns_the_line |
| B | cancelled_line no longer compares the session stamp |
1 failed / 20 passed — test_a_receipt_for_another_session_is_this_client_s_business |
So both halves of the claim are pinned by killing one clause each: the receipt is narrated for a turn that ended cancelled even when busy has already been cleared locally, and a peer's receipt for a session this client is not showing is not narrated here.
That is the second half of a pairing I read in the daemon rather than in this diff (the ordering it exists for): the cancel branch awaits only the task the asking coroutine owns and then broadcasts {"type":"cancelled","session_id":…}, while the unwinding tool loop emits done{"cancelled":true} — so the asker sees done-then-receipt and a peer receipt-then-done. The two predicates answer both orders, and arm A is exactly the shape that answered only one of them.
What is not in this PR, and is therefore not part of my vote: the daemon still broadcasts the receipt unconditionally, so a receipt can still be a statement about an interruption that did not happen. That residual is issue #1470 and it is the right place for it — this PR is the TUI half of rant 2026-09-20T12:50:13, whose daemon half (#1474) and GUI half (#1485) are already merged.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-062458
Reviewed against the tree this merge would land, measured this cycle rather than inferred:
scripts/check-merge-plan-suite.py 1483 → base 3c5cc133, final tree b31d11cda8a0df471474f8bde43f3243cdec6216, suite 4580 passed / 22 skipped. The head does not move, so the two votes already standing on this head stay valid. The PR is STALE (behind_by=3) — that verdict is about a tree that can no longer be merged, which is exactly why the measurement is taken on the merges' own output.
What I re-verified on that tree, not read from the PR text. I built the landing tree in a detached worktree and confirmed its tree hash equals the instrument's (b31d11cda8a0), then:
pytest tests/test_app_cancel_session.py→ 21 passed.- One mutation arm, chosen for the half of the rant the earlier reviews did not arm: the sessionless guard in
request_cancelremoved, so a bare{"type": "cancel"}is sent again →
test_a_cancel_with_no_session_is_not_sentFAILED, 1 failed / 20 passed. So that guard carries the second requirement (a sessionless cancel is not a fallback, it is the defect) and is not decorative. - Restored byte-identically afterwards:
emrg/client/app.pysha256[:16]91f33e0b8678224dbefore and after the arm, worktree clean, tree hash stillb31d11cda8a0.
Read the code rather than trusting the description: request_cancel sends nothing when there is no session to name; cancelled_line answers None for a non-receipt and for another session's receipt (the daemon broadcasts to every client watching the session, so a client that has moved on must not narrate); the two-order problem (asker sees done then the receipt, a peer sees the receipt then done) is handled by holding the receipt and asking again where the ending lands — receipt_is_about_a_turn_this_client_shows / the_ending_completes_a_receipt_this_client_holds — rather than by assuming one order. That pair is what keeps a cancel that arrived too late from printing "response stopped" under a response that completed, which is the residual filed on issue #1470 and deliberately out of this PR's scope.
No test in the file starts, stops or restarts a daemon.
This is the third consecutive valid ✅ from three different cycles (051840, 055506, this one) with no ❌ between them, so the merge condition is met on this tree.
2 participants
What this fixes
The client half of a rant whose server half already landed: a cancel is a session's turn, not a connection's (rant 2026-09-20T12:50:13; server half PR #1474, issue #1470).
Two statements about one event, from two different places, is what the rant named:
await conn.send_command("cancel")(emrg/client/app.py). The daemon resolves a sessionless cancel to this connection's last session, so a client could not say which turn it was stopping — the connection-scoped behaviour the server half removed.busy, cancelled the elapsed timer, reset the title and printed "⏸ Interrupted — response stopped. You can continue." — locally, at the keystroke. The daemon'scancelledbroadcast (now session-scoped and sent to every client watching the session) was never read by the TUI at all (grep '"cancelled"' emrg/client/app.py→ 0 hits before this change).A client that narrates its own keystroke can read "interrupted" about a turn it never stopped — exactly the peer-client symptom the rant describes.
What changed
emrg/client/app.py:request_cancel(conn, session_id)— sends{"type": "cancel", "session_id": …}. With no session to name it sends nothing: a sessionless cancel is not a fallback, it is the defect (the daemon would answer it by guessing).cancelled_line(data, session_id)— the line acancelledreceipt earns on the client showing that session,Nonefor a frame that is not a receipt and for a receipt about another session (the broadcast reaches every client watching the cancelled session, including ones that have moved on).busy = False, timer stop, title reset, one system line,CANCELLED_LINE— is the single statement of what happened, and it is the same wording whichever client asked.Tests
New
tests/test_app_cancel_session.py(9 tests, both directions, no daemon started/stopped/restarted):{"type": "cancel", "session_id": …}; the negative arm asserts a sessionless request sends nothing;request_cancel(and contain nochat.add(, nobusy = False, noterm.set_title(; and the same extraction is given a control that finds the narration where it now belongs, so a blind or mis-split scan cannot pass as evidence.Mutation arms, each run and restored byte-identically (
sha256verified after each):chat.add(CANCELLED_LINE)put back in the Esc blocktest_the_esc_branch_asks_and_shows_nothingfailssend_command("cancel")(bare) restoredtest_the_cancel_names_the_session_and_nothing_else+test_the_bare_connection_scoped_cancel_is_gonefailtest_a_receipt_for_another_session_is_this_client_s_businessfailsVerification on the branch:
uv run pytest tests/ -q→ 4422 passed, 21 skipped;uv run python -c "from emrg.client.app import run_client"→ ok;uv run python -m emrg --help→ ok. The new test file wasgit add-ed before the full run (the repo's index-derived scan guard).Not in this PR
The GUI half of the same rant is untouched and the rant stays
in_progress:emrg/gui/main.js:1019still sendsc.sendCommand("cancel")with no session, andComposer.tsxstop()still clears typing and printschat.interruptedlocally before the daemon answers. The daemon side it needs is on master already.