fix(gateway): kanban notifier — honor SendResult(success=False), notify on block_loop_detected, tolerate transient outages - #62712
Conversation
- honor SendResult(success=False) instead of discarding it, so an adapter that REPORTS (not raises) a soft send failure — e.g. the Telegram adapter's "Not connected" mid-reconnect — no longer advances the cursor past an undelivered event and silently loses the notification. Addresses the notifier half of NousResearch#31901. - add block_loop_detected to the notifier's TERMINAL_KINDS so a task routed to triage for a human decision (re-blocked past the recurrence limit) actually pings its subscribers instead of stalling silently. - raise MAX_SEND_FAILURES 3 -> 12 (~60s at the 5s tick) so a transient Telegram/API outage does not permanently unsubscribe a live channel now that reported soft-failures also reach this counter. - route active-profile-stamped subscriptions via the primary adapter on a single-profile gateway (self.adapters[platform] when the stamped notifier_profile equals the active profile). Related to NousResearch#56802. Adds test_kanban_notifier_rewinds_claim_on_reported_send_failure asserting a reported send failure leaves the event unseen (rewound) rather than consumed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Looks good! No obvious issues found.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Looks good! No obvious issues found.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused reliability fix. The current checkout confirms the reported premises: gateway/kanban_watchers.py:416-418 discards the SendResult, its rewind path is exception-only at :448-472, and block_loop_detected is emitted by hermes_cli/kanban_db.py:4681-4690 but omitted from TERMINAL_KINDS at gateway/kanban_watchers.py:167. The active-profile routing change also matches the primary/secondary adapter topology in gateway/run.py:8478-8491.
Problems
- The
block_loop_detecteddelivery behavior has no end-to-end notifier regression test.tests/hermes_cli/test_kanban_block_kinds.py:118-130verifies DB event emission only; it does not run the watcher or verify a subscriber receives the triage notification.
Suggested changes
- Add a notifier test that drives the recurrence limit, runs one tick, and asserts both the recording-adapter message and cursor behavior.
Automated hermes-sweeper review.
| # "status" covers dashboard drag-drop and `_set_status_direct()` | ||
| # writes — surface those transitions to subscribers too. | ||
| TERMINAL_KINDS = ("completed", "blocked", "gave_up", "crashed", "timed_out", "status", "archived", "unblocked") | ||
| TERMINAL_KINDS = ("completed", "blocked", "gave_up", "crashed", "timed_out", "status", "archived", "unblocked", "block_loop_detected") |
There was a problem hiding this comment.
Please add an end-to-end notifier regression test for this newly claimed kind: drive the block → unblock → same-cause re-block recurrence, run one watcher tick, and assert delivery plus cursor advancement. Existing coverage only confirms event emission in tests/hermes_cli/test_kanban_block_kinds.py.
…cted e2e coverage Follow-ups from review of salvaged PRs #59278 and #62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of #62712 flagged that only DB-level emission was tested).
…cted e2e coverage Follow-ups from review of salvaged PRs #59278 and #62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of #62712 flagged that only DB-level emission was tested).
|
Partially merged via PR #72236 — the block_loop_detected notification and transient-outage tolerance halves were cherry-picked with your authorship preserved. The SendResult(success=False) half had already been fixed on main (rewind-on-soft-fail landed earlier), so that part was dropped as implemented-on-main. Thanks! |
…cted e2e coverage Follow-ups from review of salvaged PRs NousResearch#59278 and NousResearch#62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of NousResearch#62712 flagged that only DB-level emission was tested).
What
Three delivery-reliability fixes to the gateway kanban notifier
(
gateway/kanban_watchers.py), plus an active-profile adapter routing fix(
gateway/authz_mixin.py):Soft send-failure was a silent drop (addresses the notifier half of [Bug]: Kanban notifications can be lost for failed sends and decomposed child tasks #31901).
The delivery loop discarded
adapter.send()'s return value. The cursor isclaimed before send, so an adapter that REPORTS failure via
SendResult(success=False)without raising (e.g. the Telegram adapter's"Not connected" mid-reconnect, or a degraded-send path) advanced the
subscription past the event — the notification was lost forever, no retry, no
log above DEBUG. Now a reported failure is raised into the existing
rewind/retry path. Regression test:
test_kanban_notifier_rewinds_claim_on_reported_send_failure.block_loop_detectednever notified. When a task re-blocks for the samecause past
BLOCK_RECURRENCE_LIMIT,block_taskroutes it totriagefor ahuman decision and emits only a
block_loop_detectedevent — a kind absentfrom the notifier's
TERMINAL_KINDS. So the one transition that exists toforce human attention produced zero notification and the task stalled in
triage silently. Added to
TERMINAL_KINDSwith a dedicated message.A brief outage permanently unsubscribed a live channel.
MAX_SEND_FAILURESwas 3 at a 5s tick — a ~15s Telegram/API blip dropped the subscription for
good (and, with fix Terminal tool #1, soft failures now reach this counter too). Raised to
12 (~60s); a genuinely dead chat still drops, just later.
Active-profile routing. A single-profile gateway stamps
notifier_profile=<active>but registers its adapter as primary;_authorization_adapternow returnsself.adapters[platform]when thestamped profile equals the active profile. Related to kanban notify/wake in multi-profile gateways: notifier coupled to dispatch_in_gateway, notifier_profile mis-resolves to 'default', wake targets dispatcher's agent instead of creator #56802 (that issue
covers the multi-profile topology; this is the single-profile facet).
Why
Each is a silent-failure path: the notifier is the only signal a human gets that
unattended work is held or failed. A dropped/blocked notification means work
piles up behind the review gate invisibly.
Testing
pytest tests/gateway/test_kanban_notifier.py tests/gateway/test_multiplex_profile_authz.py— green (16 passed), including the new regression test that asserts a reported
send failure leaves the event unseen (rewound) rather than silently consumed.
Not included
Notification-copy/UX wording is intentionally out of scope (reliability only).
The decompose-inheritance half of #31901 (child tasks not inheriting notify
subs) is not addressed here.