Skip to content

feat(notifications): bell entry when a chat turn dies - #22677

Open
dodgeblaster wants to merge 1 commit into
mainfrom
feat/kc-wt-13853
Open

dodgeblaster wants to merge 1 commit into
mainfrom
feat/kc-wt-13853

Conversation

@dodgeblaster

@dodgeblaster dodgeblaster commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

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

  • No new sound category, priority, or settings beyond the channel itself — the issue left "when / how loud" open, so this ships the minimal default-priority bell entry on its own channel; a user raises or mutes it in Settings → Notifications.
  • No notification on a turn that is auto-recovering (a retry arm) — only on the terminal ends where the next move is the operator's.
  • No bell for an app-owned chat — an app owns its chat's continuation.
  • Not auto-resume of a dead turn (that is fork PR feat(chat): resume interrupted turns on their own, within a budget #20750, which explicitly scopes this issue out).

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 except handlers 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's finally closer, 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.

Change What it does
notifications/bus.py New system.turn system channel, default priority, so a user can mute stopped-chat alerts without muting system.agent.
chat_turn/turn_reply_seams.py _notify_turn_died producer: builds the payload with payload_from_legacy(channel="system.turn", ...), sets group_key on it and pushes it on state.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 via group_key, attributed with publisher_session=slot.key so a muted session is silenced. Skips app-owned chats; swallows its own errors.
chat_turn/turn_locals.py + turn_entry.py A per-turn died_notice flag (the bell body, or None).
chat_runner.py + chat_turn/recovery.py Each terminal, non-retry arm — the stall / tool-stall "Session stuck", the exhausted transport-death give-ups, the AcpProcessDied give-up, the plain-AcpError/prompt-busy give-ups, the terminal provider-error card, and the signed-out card — sets died_notice; the finally fires 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.json Labels the channel "Chat errors" in Settings → Notifications (task language, not the ambiguous "turn"). Pseudolocale regenerated.
hooks/notificationBanner.ts A system.turn branch in the banner gate: the alert is view-targeted by its slot alone, so a /chat?sid= deep link does not match the bare /chat route and suppress the banner while the operator is on a different chat.
docs/system-specs/modules/app-notifications.md Documents the new channel, its producer, and the banner-suppression rule.

"Only when it is not the session in focus", which the issue asked for, is answered by the existing publisher_session mute 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 on system.turn with the right title, deep link (quoted), publisher_session, and group_key passed as a schema field (not meta); a TestThroughTheRealBus group pushes the output through the real legacy→bus path and asserts group_key survives 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: adds system.turn and 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"):

Settings Notifications Sources showing the new Chat errors channel

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):

Bell feed rows: a dead-turn alert and its stacked second-death variant

Related Issues

Fixes #13853

Checklist

  • At most two commits (one here), Conventional Commits title
  • Existing tests pass and new tests added
  • Self-review completed; follows project style
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5) — ✅ PASS

Design-level review of 28753fc3a85a4426727b98c7ded7223dda62d4fa — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: one flag read at a single point in finally, a new channel added without removing anything, and screenshots of the settings row and the bell row for this revision.

Notes

  • No test runs a real terminal arm through _run_chat's finally. A test that drives one arm (for example _on_acp_error_card) through to the bus would prove the single-point claim, and also that the _queue_recovery reset to None stops the bell on a turn that continues.

[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 .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5) — ✅ PASS

Premise-level review of 28753fc3a85a4426727b98c7ded7223dda62d4fa — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

First-Principles-Verdict: PASS

Nothing blocks. Each terminal arm sets one flag and a single finally site rings the bell. No existing seam already does this: inline error cards are written from 17 append("error" sites across 6 files, and no shared emitter covers them.

What this change ships

Inventory (8 items) — 8 justified

Intent: 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.

  1. A dead chat turn now puts a "⚠️ A chat stopped" entry in the bell, with a deep link to that chat. — justified
  2. A new "Chat errors" (system.turn) channel appears in Settings → Notifications, on by default, and can be muted. — justified
  3. Repeated deaths of one session stack into a single bell row (group_key). — justified
  4. A muted session's dead-turn alert is delivered silenced (publisher_session). — justified
  5. The banner still shows when you are on a different chat; it is suppressed only on the dying chat (system.turn branch in targetsCurrentView). — justified
  6. Retry arms, a user Stop, nested turns, app-owned chats and config-refusal cards (_err_meta["code"]) stay silent. — justified
  7. When a continuation gets queued, the pending bell is withdrawn (_queue_recovery clears died_notice). — justified
  8. The "Chat errors" label is added in 13 locale files, and the spec section is updated. — justified

[FIRST-PRINCIPLES-REVIEWED] 28753fc

Description read

[DESCRIPTION-READ] 6bb629c27e5f013c74fdda9960e7a1b68bda79715b97bf7715ee20549b362db9

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5) — ✅ PASS

UX-level review of 28753fc3a85a4426727b98c7ded7223dda62d4fa — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

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

  • The bell row shows the robot icon of the agent kind (_TURN_DIED_KIND = "agent"). The reader "can't tell whether the 'A chat stopped' alerts are controlled by the 'agent' switch or the 'Chat errors' switch". Give system.turn its own icon, or one that matches the "Chat errors" row.
  • The row clamps the body before the instruction, so the action is lost: "The model declined to answer and the chat stopped. Open the chat to see…". Shorten the body and lead with the action, e.g. "Model declined to answer. Open the chat to see why."
  • The bodies mix "the agent" and "the model" for what the reader takes to be one assistant. In the died_notice strings, use "the agent" throughout unless the difference matters to the user.

[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 .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 21, 2026
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review — ✅ no blocking findings

Reviewed 28753fc3a85a4426727b98c7ded7223dda62d4fa — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 28753fc

Verdict parsed from the review's SHA-scoped output markers for commit 28753fc3a85a4426727b98c7ded7223dda62d4fa.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 28753fc3a85a4426727b98c7ded7223dda62d4fa false-positive: <reason, 20 to 500 characters>

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review — ✅ no blocking findings

GPT 6.1 completed its review of 28753fc3a85a4426727b98c7ded7223dda62d4fa and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 28753fc

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 28753fc3a85a4426727b98c7ded7223dda62d4fa false-positive: <reason, 20 to 500 characters>

@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • group_key via meta is dropped by the bus

fixed in 1b52072. group_key now travels as a schema field (new param on payload_from_legacy, threaded through NotificationCoordinator.notify and state.notify), so repeated deaths of one session stack into one bell row. A real-bus test asserts group_key lands on the delivered note.

@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • Bell wired at point sites; the turn-death cause untouched (symptom-level)

fixed in 1b52072. Adopted the prescribed subtraction: the scattered per-arm calls are gone, replaced by one _notify_turn_died call at the turn's single close in the finally, driven by a per-turn died_notice flag the terminal arms set. The exception-handler deaths the goal names first (5xx/throttle give-up, AcpProcessDied) now reach the bell through that one site; a future terminal arm is covered by setting the flag.

@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • Dead-turn bell copy and the missing bell-row screenshot

Applied the copy notes in 1b52072: the channel label is now "Stopped chats" (task language, not "turn"), the bell title is "⚠️ A chat stopped", and every body leads with "Open the chat to…" so the row reads on its own. The bell-row screenshot itself cannot be captured on this build host — rendering a live notification needs a running gateway whose browser sandbox this host cannot start — so the Settings → Sources screenshot shows the new channel and the producer's exact title/body/link are pinned by the unit test instead. UX is PASS (advisory); the copy is a product-owned surface, so a maintainer may still refine the wording.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 21, 2026
@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • Dead-turn note: no slot (banner suppressed on any chat page) and kind="turn" (wrong sound category)

fixed in 6b6d1eb. (1) The note now carries meta={"slot": slot.key}, and targetsCurrentView gains a system.turn branch that is view-targeted by slot alone — so a /chat?sid= link no longer matches the bare /chat route and the banner shows for a death in a DIFFERENT chat. (2) The legacy kind is now "agent", not "turn", so the dead-turn alert is governed by its own system.turn channel rather than the conversation-ready chime category (presetForKind). Also documented system.turn in docs/system-specs/modules/app-notifications.md. Tests added: NotificationBanner.test.tsx pins the slot-only targeting; test_turn_died_notify.py pins kind=agent and the slot meta.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 21, 2026
@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • Pipe death arriving as a plain AcpError + exhausted budget rang no bell

fixed in 9c9eb2b. Set turn.died_notice in both branches of the AcpError _exhausted give-up arm (chat_runner.py), matching the typed-AcpProcessDied sibling: own-fault → "connection kept dropping", shared → "process kept restarting", with a neutral body for the prompt-busy (non-pipe-death) case that shares the arm. Also covered the remaining terminal give-up in recovery.py::_requeue_after_prompt_busy (prompt-busy budget exhausted) by threading turn and setting died_notice there. Both are past the Stop-suppression guard, so a user cancel never rings. Tests: composition contract + producer green; test_chat_requeue_resolved_stop green.

@dodgeblaster

Copy link
Copy Markdown
Contributor Author

Rebased: main moved (85a0817 -> cb604a4) since the last head; CI re-running on 9c9eb2b. No PR files conflicted.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 21, 2026
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Oct 21, 2026
@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • Generic terminal arm (AcpRequestTimeout/AcpSessionStartTimeout) rang no bell; publisher_session named the wrong key for an unbound channel slot

fixed in bb13c9c. (1) The generic except Exception terminal arm (chat_runner.py ~6905) now sets turn.died_notice for a genuine runtime death — a shared-runtime prompt/session-start timeout (AcpRuntimeError subclasses that skip the AcpError arms) or a plain terminal Exception — while deliberately skipping the deterministic config-refusal cases (memory_unavailable, capability/session-start refusals carry their own guided-fix card, keyed by _err_meta['code']). (2) publisher_session now uses slot.key, not the effective session key: session_is_muted resolves the publisher through caller_slot_key, which never matches an unbound channel-born slot's dashboard:slack_<ts> key — so the mute now applies. Same key member_message_notes publishes under. Tests updated to pin publisher_session=slot.key and the group/publisher split.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 21, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 21, 2026
@dodgeblaster

Copy link
Copy Markdown
Contributor Author
  • "Stopped chats" label was ambiguous; bell/stacked states were unshown

fixed in 4d3dded. Renamed the channel label to "Chat errors" (says failure, not user-stopped/finished), per the ask. Closed two evidence gaps with a new attachment: the bell-feed row ("⚠️ A chat stopped" + body + unread dot) and the stacked second-death variant ("2 in this chat") rendered by the real NotificationCard with a staged system.turn note. The banner on another chat is the SAME NotificationCard surface; its slot-only view-targeting is pinned by NotificationBanner.test.tsx (targetsCurrentView returns false for a system.turn note whose slot != the active chat). A live dying-turn capture needs a running gateway whose browser sandbox this build host cannot start, so the producer's exact title/body/link/group_key/slot are pinned by test_turn_died_notify.py instead.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 21, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 21, 2026
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Oct 22, 2026
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Oct 26, 2026
@dodgeblaster

Copy link
Copy Markdown
Contributor Author

Backend Tests shard 4 (Windows, and 3.12 where it shows) is red on test/test_telegram_dispatcher.py::TestABusyReplyCarriesItsContext (test_a_declined_steer_with_an_injection_parent_audits_once_at_the_queue, test_an_injection_reply_on_the_queue_path_audits_exactly_once). This is a pre-existing main red tracked in #24514 (fix in flight), not introduced by this PR — this PR's diff does not touch the Telegram dispatcher or any messaging code. Treating it as a named human-gated red; a Coverage Gate failing closed solely on that shard's backend-test=failure is its downstream cascade.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 26, 2026
@dodgeblaster
dodgeblaster marked this pull request as ready for review October 27, 2026 16:01
@dodgeblaster
dodgeblaster requested review from a team and KiroCrewCaptain as code owners October 27, 2026 16:01

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No notification when a turn dies (provider 5xx / throttle / content filter) -- only the inline Resume error

1 participant