fix: clarify cancelled chat turn status

[Jordan-SkyLF]

Thinking Path

Small, focused bug fix. Explicit user cancellation should not be reported as a provider/no-content failure, and the UI should keep the same verbose error-card shape users already see for real provider errors.

What Changed

• Classify user/client cancellation, interruption/abort, provider-empty/no-content, and provider/rate/quota errors separately in streaming error handling.
• Persist cancelled turns as _error assistant markers with verbose copy and a Cancellation details disclosure, so reloads match the live UI.
• Reuse the existing provider-error/details rendering with a custom details label instead of adding a new UI component.
• Add race/idempotency guards so:
  • worker finalization and /api/chat/cancel do not duplicate cancel markers,
  • late Stop clicks after a completed worker save do not emit contradictory cancel events,
  • partial streamed text/reasoning/tool-call metadata is still preserved on real cancellation.
• Keep cancellation/error markers out of the next provider request via the existing _error sanitization path.

Why It Matters

Previously, cancelled or interrupted turns could look like provider failures, especially the misleading No response from provider path. That makes normal user cancellation look like an upstream/model problem and can persist incorrect state across reloads.

This keeps the user-visible format familiar while making the status accurate:

Situation	Display
User cancelled	Task cancelled + Cancellation details
Interrupted/aborted	Response interrupted + Interruption details
Provider returned no content	No response from provider + Provider details
Provider/rate/quota/auth/model errors	Existing provider-specific error labels/details

Verification

• git diff --check
• python -m py_compile api/streaming.py
• node --check static/messages.js
• node --check static/ui.js
• Targeted regression suites:
  • python -m pytest tests/test_issue1361_cancel_data_loss.py tests/test_cancelled_turn_status.py tests/test_issue893_cancel_preserves_partial.py -q
  • 33 passed
• Full local suite:
  • python -m pytest tests/ -q --tb=short
  • 5322 passed, 4 skipped, 1 xfailed, 2 xpassed, 8 subtests passed in 154.90s
• Manual browser smoke on a local WebUI instance:
  • started a streaming turn,
  • clicked Stop/Cancel,
  • confirmed verbose Task cancelled: output,
  • confirmed Cancellation details,
  • confirmed no No response from provider,
  • confirmed no browser console errors.
• Static secret scan of the PR diff: {}.
• Follow-up review-feedback patch:
  • added string-only <CancelledError> classification coverage,
  • centralized cancel-marker substring matching via _CANCEL_MARKER_PATTERNS,
  • GitHub PR checks passed for Python 3.11, 3.12, and 3.13 on follow-up commit 112eadc.

Screenshots / UI Notes

This is a small copy/status/details-label change that intentionally reuses the existing provider-error UI. Manual browser smoke verified the after-state; no new interface component or layout was introduced.

Risks / Follow-ups

• Source-substring tests are used in a few places as structural guards around streaming race paths. They are intentional but may need adjustment if api/streaming.py is later refactored.
• No new dependencies.

Model Used

AI-assisted with Hermes/Skyly using OpenAI gpt-5.5. An independent AI review pass was also run before opening the PR; it reported no blocking issues or security concerns.

[nesquena-hermes]

Reading the diff for api/streaming.py (+334 lines), static/messages.js, static/ui.js, and the new tests/test_cancelled_turn_status.py, this is a non-trivial but appropriately surgical change. The classification + finalization helpers are well-factored; my comments focus on a few race ordering and ephemeral-session edges I'd want a second pair of eyes on before merge.

Classification helper (api/streaming.py:139-176)

_is_cancelled = (
    'cancelled by user' in _err_lower
    or 'canceled by user' in _err_lower
    or ...
    or (exc is not None and type(exc).__name__ in ('CancelledError', 'CanceledError'))
)

Order matters here — _is_cancelled is checked before _is_quota, _is_auth, _is_rate_limit. This is the right priority: if the user pressed Stop and the provider also returned a 429 in flight, the user's intent should win the label. Good.

One missed signal: asyncio.CancelledError in Python 3.8+ inherits from BaseException, not Exception, and the agent loop may surface it via agent._last_error as "<CancelledError>" repr-only. The current type(exc).__name__ check catches it correctly when the exception object is available, but the string match on _err_lower won't match a bare "CancelledError" repr. Probably fine — the calling sites pass exc — but worth one literal-substring 'cancellederror' in _err_lower for the string-only path.

Persistence guard (api/streaming.py:248-272)

_session_has_cancel_marker walks messages backwards, stops at the first user role, and matches on lowercased "task cancelled"/"task canceled"/"response interrupted". That short-circuit on user is the right shape — it scopes the dedupe check to the current turn, not the whole conversation history.

for msg in reversed(getattr(session, 'messages', None) or []):
    if not isinstance(msg, dict): continue
    if msg.get('role') == 'user': return False
    if msg.get('role') != 'assistant': continue
    ...

One concern: _cancelled_turn_content produces a Markdown-bold prefix "**Task cancelled:**", but the dedupe match is 'task cancelled' in normalized (lowercased, asterisks intact in normalized). The asterisks don't break the substring match, so this works. But if anyone later changes the prefix to use a different marker (e.g. emoji), this string match silently breaks. Worth a constant: _CANCEL_MARKER_PATTERNS = ('task cancelled', 'task canceled', 'response interrupted').

Race ordering in cancel_stream (api/streaming.py:~4453, ~4508)

The new guard is the right shape:

if (getattr(_cs, 'active_stream_id', None) != stream_id
        and not getattr(_cs, 'pending_user_message', None)):
    _emit_cancel_event = False
    return True

This handles "Stop pressed after worker already saved + cleared stream state." But notice the ordering — _emit_cancel_event is captured before the lock is acquired, then q.put_nowait(('cancel', ...)) is deferred to after the with _get_session_agent_lock(_cancel_session_id): block. The worker's _finalize_cancelled_turn path also calls put('cancel', ...) inside _agent_lock. If both paths win the lock back-to-back, you can still emit two cancel SSE frames to the client (worker's, then cancel_stream's, if _emit_cancel_event wasn't flipped to False).

The guard depends on _cs.active_stream_id != stream_id to flip the flag. The worker's cancel paths clear active_stream_id via _finalize_cancelled_turn → _persist_cancelled_turn (api/streaming.py:283):

session.active_stream_id = None
session.pending_user_message = None

So after the worker's _finalize_cancelled_turn lands, a subsequent cancel_stream call will correctly see active_stream_id is None != stream_id and suppress. Good. But during the narrow window where the worker is mid-_finalize_cancelled_turn (between _persist_cancelled_turn writing fields and put('cancel', ...) emitting), cancel_stream's guard could miss it. The with _agent_lock around both paths serializes the mutation, so by the time cancel_stream enters _agent_lock, the worker's mutation has fully landed. ✓ The lock makes this safe.

Ephemeral path (/btw)

_cleanup_ephemeral_cancelled_turn (api/streaming.py:286-297) does pathlib.Path(session.path).unlink(missing_ok=True). session.path is a Path already (api/models.py:454), and Session.save() writes via tmp+rename, so the unlink is idempotent. Fine. But there's no test that exercises the /btw codepath end-to-end here — test_ephemeral_cancel_finalizer_unlinks_temp_session_without_saving_error_marker only tests the helper in isolation. Worth a follow-up integration test once /btw has stable fixtures.

Frontend (static/messages.js:1228-1239, static/ui.js:5013-5017)

The non-streaming apperror fallback now picks up Cancellation details / Interruption details labels, and renderMessages honors provider_details_label||'Provider details'. The fallback in the catch arm of the cancel handler (static/messages.js:1326-1329) now writes the full verbose marker rather than the bare '*Task cancelled.*' — so the API-failure path and the SSE path produce identical UI. Good consistency.

Verdict

The diff is large but the design is sound: classify first, finalize centrally, dedupe via session-state inspection, guard the cancel-stream emission. I'd merge this once the maintainer confirms the manual smoke (Stop mid-stream, Stop after completion, /btw cancel) all behave. The constant-extraction for cancel-marker substrings would be a nice tidy-up post-merge.

Tests look complete: test_cancelled_turn_status.py exercises classification, both finalize variants, and pins the source-substring guards on streaming.py blocks (silent-failure, exception path, post-merge cancel guard). 31 passed + 5322 full-suite passed in the PR description is consistent with the diff surface.

[bergeouss]

Review Feedback Addressed

• Issue: Reviewer noted the string-only classification path misses bare CancelledError repr, and cancel-marker substrings are duplicated across 3 locations without a shared constant
• Fix: Added 'cancellederror' in _err_lower substring check in _classify_provider_error. Extracted _CANCEL_MARKER_PATTERNS = ('task cancelled', 'task canceled', 'response interrupted') constant and replaced all 3 inline occurrences with any(p in ... for p in _CANCEL_MARKER_PATTERNS)
• Files: api/streaming.py

🤖 AI-assisted via Hermes Agent

[Jordan-SkyLF]

Addressed the review feedback in follow-up commit 112eadc:

• added string-only <CancelledError> / cancellederror classification coverage so that path is treated as user cancellation rather than provider no-response
• extracted _CANCEL_MARKER_PATTERNS and reused it for cancellation/interruption marker dedupe
• reran targeted checks locally: git diff --check, python -m py_compile api/streaming.py, node --check static/messages.js, node --check static/ui.js, and the targeted pytest suite: 33 passed
• GitHub PR checks are green for Python 3.11, 3.12, and 3.13

[Jordan-SkyLF]

This closure was accidental. The branch has been restored and PR #2151 has been reopened so review can continue.