Repository navigation
feat(notifications): bell entry when a chat turn dies - #22677
dodgeblaster wants to merge 1 commit into
Conversation
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: one flag read at a single point in Notes
[DESIGN-REVIEWED] 28753fc Description read[DESCRIPTION-READ] b2c6c5c1986b37d039cc04a3c94395d170cd66e2f591be187420477e26a2b742 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. Each terminal arm sets one flag and a single What this change shipsInventory (8 items) — 8 justifiedIntent: Let an operator who is working in another session learn that a chat turn died with an error they must act on (#13853). This is an ADDITION.
[FIRST-PRINCIPLES-REVIEWED] 28753fc Description read[DESCRIPTION-READ] 6bb629c27e5f013c74fdda9960e7a1b68bda79715b97bf7715ee20549b362db9 That sha256 names the PR description this verdict read. Recompute it with |
UX Review (Opus 5.5) — ✅ PASSUX-level review of UX-Verdict: PASS Nothing blocks. The blind reader correctly read both primary surfaces: the "A chat stopped" bell row ("opens that chat so I can try again… yes") and the "Chat errors" source row. Notes
[UX-REVIEWED] 28753fc Description read[DESCRIPTION-READ] 2857e601225f6a44ba38f838b1f07c97791285870e79c503f5d43021c6390bfb That sha256 names everything this verdict read: the PR description, plus the 5 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Opus 5.5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 6.1 Review — ✅ no blocking findingsGPT 6.1 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
226f76c to
1b52072
Compare
|
|
|
1b52072 to
97c1857
Compare
97c1857 to
6b6d1eb
Compare
|
6b6d1eb to
9c9eb2b
Compare
|
52380fd to
bb13c9c
Compare
|
bb13c9c to
a099d11
Compare
a099d11 to
4d3dded
Compare
|
4d3dded to
a0089b6
Compare
a0089b6 to
d739112
Compare
d739112 to
4daedc2
Compare
4daedc2 to
28753fc
Compare
|
Backend Tests shard 4 (Windows, and 3.12 where it shows) is red on |
Problem / Motivation
Goal: Ring the bell when a chat turn dies with an error the user must act on, so an operator working in another session learns work has stopped without opening it.
When a turn ends in an error — a provider 5xx / throttle, a content-filter refusal, or the connection to the agent dropping for good — the transcript shows the inline error card with its Resume / "start a new chat" control, but nothing reaches the notification bell. An operator who switched to another session finds out only by opening the dead one.
Why it matters
A long turn can die while the operator is elsewhere. Today the only trace is the inline card, so the session sits stopped and unnoticed until someone happens to look. A bell entry turns "discover it later" into "told right away", using the surfaces the user already tuned (bell, sound, banner).
Not a goal
What changed (motivation → approach → change)
A dead turn already writes an inline error card at many terminal arms in the chat runner — some on the stop-reason path, some in the
excepthandlers that catch the most common provider / transport deaths — but no producer ever reached the notification bus, so the bell stayed silent. The fix rings the bell at a single choke point in the turn'sfinallycloser, so every terminal arm rings through one site and a future terminal arm is covered by setting a flag rather than remembering a scattered call.notifications/bus.pysystem.turnsystem channel, default priority, so a user can mute stopped-chat alerts without mutingsystem.agent.chat_turn/turn_reply_seams.py_notify_turn_diedproducer: builds the payload withpayload_from_legacy(channel="system.turn", ...), setsgroup_keyon it and pushes it onstate.notification_bus— the way every other legacy producer stacks a note. A default-priority bell entry with a deep link to the session, deduped per session viagroup_key, attributed withpublisher_session=slot.keyso a muted session is silenced. Skips app-owned chats; swallows its own errors.chat_turn/turn_locals.py+turn_entry.pydied_noticeflag (the bell body, or None).chat_runner.py+chat_turn/recovery.pyAcpProcessDiedgive-up, the plain-AcpError/prompt-busy give-ups, the terminal provider-error card, and the signed-out card — setsdied_notice; thefinallyfires the producer once. Retry arms, branches reached only on a user Stop, and the nested-turn (_prompt_depth > 0) arm whose parent turn may absorb the death all stay silent.NotificationsPanel.tsx+en.jsonhooks/notificationBanner.tssystem.turnbranch in the banner gate: the alert is view-targeted by its slot alone, so a/chat?sid=deep link does not match the bare/chatroute and suppress the banner while the operator is on a different chat.docs/system-specs/modules/app-notifications.md"Only when it is not the session in focus", which the issue asked for, is answered by the existing
publisher_sessionmute rather than new per-client focus tracking: a session the user has muted has its alert silenced.Backwards compatibility
Compatible: adds a new notification channel, a new producer. No existing channel, note shape, caller, or path changes.
Removes nothing: the change only adds a channel a producer, and a settings label.
Tests
test/test_turn_died_notify.py(new): the producer lands one note onsystem.turnwith the right title, deep link (quoted),publisher_session, andgroup_keypassed as a schema field (not meta); aTestThroughTheRealBusgroup pushes the output through the real legacy→bus path and assertsgroup_keysurvives onto the delivered note (the regression the earlier kwargs-only test missed); skips an app-owned chat; swallows a delivery failure; omits group/publisher when the session key is blank.website/src/test/NotificationsPanel.channels.test.tsx: addssystem.turnand asserts it renders as "Chat errors", not the bare id.Manual verification
Settings → Notifications → Sources renders the new "Chat errors" channel with its mute toggle and priority override (screenshot below). The bell-on-death path is covered by the producer unit test and by the terminal arms setting
died_notice; an end-to-end capture of a dying turn needs a running gateway with a browser sandbox this build host cannot start, so the producer is proven by unit test and the UI surface by the rendered panel.Screenshots / video
The new channel in Settings → Notifications → Sources (labelled "Chat errors"):
The bell-feed row a dead turn produces — the alert itself, and the stacked state when one chat dies twice (shared
group_key, one row):Related Issues
Fixes #13853
Checklist