From d39d30a2139afc13259b34d3b99f0157ef071577 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 22 Apr 2026 22:17:40 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20correct=20message=20ordering=20after=20t?= =?UTF-8?q?ask=20cancellation=20=E2=80=94=20v0.50.163=20(#883)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix: correct message ordering after task cancellation — v0.50.163 (#883) Fixes the message-ordering glitch from #882: clicking Cancel while the agent is responding could cause a subsequent response to render above the "*Task cancelled.*" marker. Root cause: the cancel handler pushed the marker only to local S.messages without persisting to the server. When the done event fired shortly after and replaced S.messages from server state, the marker disappeared from client state while the next response anchored to the server-authoritative position. Fix has three parts: - Server (cancel_stream): append *Task cancelled.* to session.messages with _error:True + timestamp, then save. _error ensures _sanitize_messages_for_api() strips it from conversation_history on the next agent turn, so the LLM never sees it as a prior assistant turn. Precedent: same flag used for the apperror marker at line 1343. - Client (SSE cancel handler): fetch /api/session instead of pushing locally (same pattern as the done handler). Falls back to local push if the fetch fails. - Tests: fix test window width for cancel handler (1200→dynamic); add two regression tests pinning _error flag and _sanitize invariant. 1941 tests passing. Co-authored-by: piliang --- CHANGELOG.md | 11 +++++++++ api/streaming.py | 12 +++++++++ static/messages.js | 24 +++++++++++++++--- tests/test_sprint36.py | 56 ++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c3f8aa..9265d8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ workspace subtree) and never enumerate blocked system roots. (`api/routes.py`, `api/workspace.py`, `static/panels.js`, `static/style.css`) (partial for #616) +## [v0.50.163] — 2026-04-23 + +### Fixed +- **Message ordering after task cancellation** — cancelling a stream while the + agent is responding no longer causes subsequent responses to appear above the + "Task cancelled." marker. The cancel handler now fetches the authoritative + message list from the server (same as the done event), and the server persists + the cancel message to the session so both paths stay in sync. Falls back to + the previous local-push behaviour if the API call fails. (`api/streaming.py`, + `static/messages.js`) (@mittyok, #882) + ## [v0.50.161] — 2026-04-23 ### Fixed diff --git a/api/streaming.py b/api/streaming.py index bd67f4d..97d0d89 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -1666,6 +1666,18 @@ def cancel_stream(stream_id: str) -> bool: _cs.pending_user_message = None _cs.pending_attachments = [] _cs.pending_started_at = None + # Add cancel message to session messages so client sees consistent state. + # _error=True flags this as a synthetic UI marker so + # _sanitize_messages_for_api() (line 591-593) strips it from the + # conversation_history passed to the agent on the NEXT user message — + # otherwise the model would see "Task cancelled." in its history as a + # prior assistant turn and could respond accordingly. + _cs.messages.append({ + 'role': 'assistant', + 'content': '*Task cancelled.*', + '_error': True, + 'timestamp': int(time.time()), + }) _cs.save() except Exception: logger.debug("Failed to clear session state on cancel for %s", _cancel_session_id) diff --git a/static/messages.js b/static/messages.js index ca7e9dd..d2d8d95 100644 --- a/static/messages.js +++ b/static/messages.js @@ -653,10 +653,26 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null;const _cbc=$('btnCancel');if(_cbc)_cbc.style.display='none'; } - if(S.session&&S.session.session_id===activeSid){ - clearLiveToolCards();if(!assistantText)removeThinking(); - S.messages.push({role:'assistant',content:'*Task cancelled.*'});renderMessages(); - } + // Fetch latest session from server to get accurate message list (includes cancel status) + // This ensures messages stay in sync with server, fixing race condition where local + // "*Task cancelled.*" message gets lost when done event overwrites S.messages + (async()=>{ + try{ + const data=await api(`/api/session?session_id=${encodeURIComponent(activeSid)}`); + if(data&&data.session&&S.session&&S.session.session_id===activeSid){ + S.session=data.session; + S.messages=(data.session.messages||[]).filter(m=>m&&m.role); + clearLiveToolCards();if(!assistantText)removeThinking(); + renderMessages(); + } + }catch(_){ + // Fallback to local cancel message if API fails + if(S.session&&S.session.session_id===activeSid){ + clearLiveToolCards();if(!assistantText)removeThinking(); + S.messages.push({role:'assistant',content:'*Task cancelled.*'});renderMessages(); + } + } + })(); renderSessionList(); if(!S.session||!INFLIGHT[S.session.session_id]){setBusy(false);setComposerStatus('');} }); diff --git a/tests/test_sprint36.py b/tests/test_sprint36.py index 67cd7df..bf058ed 100644 --- a/tests/test_sprint36.py +++ b/tests/test_sprint36.py @@ -158,11 +158,13 @@ def test_sse_cancel_handler_still_present(): def test_sse_cancel_handler_calls_set_busy(): """The SSE cancel handler must still call setBusy(false).""" src = read("static/messages.js") - idx = src.find("addEventListener('cancel'") + idx = src.find("addEventListener('cancel'") if idx == -1: idx = src.find('addEventListener("cancel"') assert idx != -1 - block = src[idx:idx + 1200] + # Find the closing of this handler block (next top-level addEventListener) + next_handler = src.find("source.addEventListener(", idx + 50) + block = src[idx:next_handler] if next_handler != -1 else src[idx:idx + 3000] assert "setBusy(false)" in block, ( "SSE cancel handler no longer calls setBusy(false)" ) @@ -180,3 +182,53 @@ def test_cancel_failed_i18n_key_exists_in_all_locales(): f"cancel_failed key only found {count} times in i18n.js — " f"expected at least {locale_count} (one per locale)" ) + + +# ── 8. Server-persisted cancel marker doesn't leak into agent history ──────── + +def test_cancel_marker_flagged_as_error_to_skip_in_api_history(): + """The server-side cancel marker appended in cancel_stream() must carry + _error: True so _sanitize_messages_for_api() strips it from the + conversation_history sent to the agent on the next user message. + + Without this flag, the LLM sees "*Task cancelled.*" as a prior assistant + turn and may reference it in subsequent responses ("As I mentioned, I was + cancelled...") — a behavioral regression introduced when this PR started + persisting the marker to the session. + """ + src = read("api/streaming.py") + idx = src.find("'content': '*Task cancelled.*'") + if idx == -1: + idx = src.find('"content": "*Task cancelled.*"') + assert idx != -1, "cancel marker content string not found in cancel_stream()" + + # Walk back to the start of the dict literal (opening brace) + brace_open = src.rfind("{", 0, idx) + brace_close = src.find("}", idx) + assert brace_open != -1 and brace_close != -1, "couldn't locate cancel marker dict" + + marker_dict = src[brace_open:brace_close + 1] + assert "_error" in marker_dict and "True" in marker_dict, ( + "cancel marker is missing _error: True — it will leak into the agent's " + "conversation_history via _sanitize_messages_for_api() on the next turn. " + "See line 591-593 of api/streaming.py for the error-marker filter." + ) + + +def test_sanitize_strips_error_flagged_assistant_messages(): + """_sanitize_messages_for_api() must drop messages with _error: True — + this is the invariant the cancel marker's _error flag relies on.""" + from api.streaming import _sanitize_messages_for_api + messages = [ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi"}, + {"role": "assistant", "content": "*Task cancelled.*", "_error": True}, + {"role": "user", "content": "next"}, + ] + sanitized = _sanitize_messages_for_api(messages) + assert len(sanitized) == 3, ( + f"expected 3 messages (cancel marker stripped), got {len(sanitized)}: {sanitized}" + ) + assert all("Task cancelled" not in (m.get("content") or "") for m in sanitized), ( + "_sanitize_messages_for_api must filter cancel markers from API history" + )