feat(gateway): per-platform gateway_restart_notification_channel override

[julio-cloudvisor]

What this adds

An optional per-platform gateway_restart_notification_channel. When set, all lifecycle notifications for that platform are routed to that single chat instead of the active-session sources and the home channel — so operators can watch restarts in an ops channel (e.g. #ops-alerts) without pinging end-user channels during active conversations.

Rework in response to review

Thanks for the detailed review — every point below is addressed. The branch was rebased onto current main first; several of the cited line numbers had moved and the kanban change had already landed, so I resolved against the current source and note the deltas inline.

Review asks → changes

• Check SendResult.success, not just non-throw. Override delivery now goes through a single helper _send_lifecycle_override_notification() that returns True only when the adapter does not report success=False. On failure it returns False and the caller falls back to the normal per-session / home-channel delivery — no more silently-claimed delivery. Mirrors the existing lifecycle success=False checks in _notify_active_sessions_of_shutdown / _send_restart_notification.
  Test: test_override_send_failure_falls_back_to_active_session.

• Cover ALL lifecycle paths. The override now routes every lifecycle path (and only it, when a send succeeds):

  • shutdown active-session pings — _notify_active_sessions_of_shutdown (per-platform skip set)
  • shutdown home-channel broadcast — same function, second loop now also skips override platforms
  • post-restart "restarted" ping — _send_restart_notification
  • startup "online" ping — _send_home_channel_startup_notifications
    Tests: test_shutdown_active_session_routed_to_override_channel, test_shutdown_home_channel_routed_to_override_channel, test_restart_notification_routed_to_override_channel, test_startup_notification_routed_to_override_channel, plus test_override_suppressed_when_restart_notification_flag_false (the existing gateway_restart_notification=False suppression still wins).

• Wire the new field into the shared-key config bridge. Added the gateway_restart_notification_channel bridge line next to gateway_restart_notification in the shared-key loop in load_gateway_config(), and the from_dict fallback reads it from extra too (mirrors _grn / typing_indicator).
  Tests (both shapes): test_bridges_gateway_restart_notification_channel_top_level_key (top-level shared key) and test_bridges_gateway_restart_notification_channel_nested_platforms (nested platforms: block), plus unit-level from_dict / to_dict roundtrip and ..._resolved_from_extra.

• Platform-safe target encoding. Dropped the :-split entirely. The configured value is used verbatim as the chat id, matching how home_channel.chat_id is treated, so a Matrix room id like !room123:example.org survives intact. Thread/topic targeting is not encoded into this single string precisely because platform ids can contain :; point the override at a channel/room directly.
  Tests: test_override_channel_matrix_room_id_not_split_on_colon (runtime path) and test_gateway_restart_notification_channel_matrix_room_id_preserved (config roundtrip).

• Drop the unrelated kanban PRAGMA change. Removed from the diff entirely (it had already landed on main at hermes_cli/kanban_db.py). The PR is now single-purpose: gateway/config.py, gateway/run.py, and their tests only.

Notes on cited line numbers

main moved since the review: typing_indicator / channel_overrides were added to PlatformConfig and the shutdown path now builds metadata via _thread_metadata_for_target(...) and already checks SendResult.success. I integrated with those rather than the older shapes, so the concrete line numbers differ from the review — the function names above are the stable anchors.

Tests

pytest tests/gateway/ for the touched areas: new override file 7 passed, new config cases 7 passed, full test_config.py + test_restart_drain.py + test_restart_notification.py + test_gateway_shutdown.py + test_typing_indicator_toggle.py + the new file = 186 passed, and the broader restart/startup/shutdown/lifecycle/drain/notification selection = 584 passed / 4 skipped, no regressions.

[jsboige]

PR Review — #31046

Summary

Adds an optional gateway_restart_notification_channel per-platform config field that routes restart/shutdown notifications to a specific chat (ops channel) instead of broadcasting to every active session source. Also includes an unrelated SQLite durability fix in kanban_db.py.

Author: julio-cloudvisor | Branch: cv/gateway-lifecycle-notify-channel-config -> main
Files changed: 3 | +88 / -1

CI Status

No CI checks reported on this branch.

---

Issues Found

WARNING (3)

File	Issue	Recommendation
hermes_cli/kanban_db.py	Unrelated change bundled into a gateway feature PR. The synchronous=NORMAL->FULL + wal_autocheckpoint=100 change fixes issue #30896 (database corruption under concurrent writes). This is a distinct bugfix with its own risk profile and should be a separate PR. Mixing it here means a reviewer approving the gateway feature implicitly approves a SQLite durability change without proper scrutiny.	Split into a separate PR referencing #30896. The gateway feature diff is clean on its own.
hermes_cli/kanban_db.py	synchronous=FULL + wal_autocheckpoint=100 performance impact is asserted but unmeasured. The comment says "performance cost is bounded because the kanban workload is dispatch-bounded" -- this should be backed by a benchmark or at minimum a measurement of write throughput before/after. FULL forces fsync per commit, and checkpointing every 100 pages (vs 1000 default) adds more frequent WAL rewriting.	Provide benchmark data or at minimum document observed dispatch latency before/after on a representative workload.
(no tests)	No unit tests for the new feature. The test plan in the PR body is reasonable but unexecuted. The shutdown notification path is a critical lifecycle operation -- a bug here means operators do not get notified of restarts, or customer-facing channels get spurious pings. The existing gateway_restart_notification=False suppression test should be mirrored for the override path.	Add at least a unit test asserting: (1) override set -> single send to override chat, 0 per-session sends on that platform; (2) override send fails -> per-session loop still runs as fallback; (3) override set but gateway_restart_notification=False -> no sends at all.

INFO (2)

File	Suggestion
gateway/run.py L3202	getattr(platform_cfg, "gateway_restart_notification_channel", None) is unnecessarily defensive. The field has a default value on PlatformConfig, so direct attribute access (platform_cfg.gateway_restart_notification_channel) works and is consistent with the platform_cfg.gateway_restart_notification access two lines above.
gateway/run.py L3217	When the override send fails, the per-session loop still runs as a fallback (platform is not added to override_sent_platforms). This is a reasonable silent fallback, but it is worth a brief comment noting this intentional behavior, so future readers understand it is not a bug.

---

Positive Points

• Clean backwards compatibility: field defaults to None, existing deployments unaffected.
• from_dict follows the existing pattern for gateway_restart_notification (top-level + extra-bridge keys).
• to_dict omits the field when None, keeping config output clean.
• Thread ID parsing (split(":", 1)) is correct for Slack-style composite IDs.
• Error handling in the override send path is consistent with the existing per-session loop (best-effort, logged, non-blocking).
• The override/fallback behavior (override send fails -> per-session loop still runs) is a sensible degradation strategy.

Recommendation

• COMMENT — The gateway feature implementation is sound and well-structured, but I would recommend:
  1. Split out the kanban_db.py change into its own PR (it is a separate fix with its own risk profile).
  2. Add unit tests before merge (the shutdown notification path is lifecycle-critical).
  3. The two INFO items are optional cleanups.

The gateway code itself is well-written with good comments, clean backwards compat, and a sensible fallback. My hesitation is primarily about the mixed scope and missing tests, not the implementation quality.

[teknium1]

Thanks for the focused gateway-routing proposal. The underlying problem remains on current main: active-session shutdown notices are still sent per target at gateway/run.py:5806.

Problems

• gateway/run.py:3205 treats every non-throwing adapter.send() as delivered. The adapter contract returns SendResult; current lifecycle paths check success=False before suppressing fallback (gateway/platforms/base.py:1878-1883, gateway/run.py:5806-5816).
• The override only bypasses active-session shutdown notices. Home-channel shutdown delivery remains at gateway/run.py:5862-5890, and startup/restart delivery remains at gateway/run.py:7300-7311 and 14828-14964, so the claimed single lifecycle notification is not guaranteed.
• The new field is not added to the existing shared-key bridge at gateway/config.py:1239-1242, so the supported top-level per-platform config shape will not propagate it.
• Splitting all targets on : breaks Matrix-style room IDs; current coverage explicitly exercises !room123:example.org in tests/gateway/test_restart_drain.py:515-538.
• The unrelated Kanban PRAGMA change is already implemented on main at hermes_cli/kanban_db.py:1729-1733 and 1761-1768.

Suggested changes

• Check SendResult.success, cover all intended lifecycle paths, bridge and test both config shapes, use platform-safe target encoding, and omit the already-landed Kanban change.

Automated hermes-sweeper review.

[teknium1]

adapter.send() may return SendResult(success=False) rather than raise. Please inspect that result before adding this platform to override_sent_platforms; otherwise a rejected override delivery suppresses the existing active-session fallback.

[teknium1]

This generic colon split is not safe for every platform: Matrix room IDs themselves contain : (see the current Matrix shutdown-notification regression in tests/gateway/test_restart_drain.py). Use a platform-aware or unambiguous target representation.

[teknium1]

Please also add this key to the shared-key bridge in load_gateway_config() beside gateway_restart_notification. This from_dict() fallback alone does not propagate the existing top-level per-platform config shape.

[julio-cloudvisor]

Thanks for the review @jsboige, and apologies for the long silence on this one.

I have addressed both points:

• The unrelated SQLite durability change is gone. Upstream shipped its own (better) durability work in the meantime, so the branch now contains only the feature: the per-platform gateway_restart_notification_channel override, its config plumbing, and tests.
• Rebased onto current main. The restart-notification area was reworked since I opened this (the per-platform gateway_restart_notification boolean now exists natively), so I re-anchored the channel override on top of that: the boolean controls whether restart notices are sent, and this field controls where they go. That combination is exactly what we need in production: we run 8 gateways and want deploy-window restart notices routed to a single ops channel instead of broadcast into every active session's channel.

Test status after the rebase: tests/gateway/test_restart_notification_channel_override.py (new) and tests/gateway/test_config.py pass locally, except two pre-existing test_slack_*_env_bridge failures that also fail for me on unpatched main, so they are unrelated to this change.

Would appreciate another look when you get a chance.

[GottZ]

This was generated by AI during triage.

Summary

Five PRs address or reference restart-notification targeting. #20801/#20892 provide the merged per-platform kill switch, #31046 routes every lifecycle notification to one operator channel, #52584 selectively suppresses bare channels while preserving DMs and threads, and #56015 gates existing targets through an allowlist rather than rerouting them.

Related pull requests

• #20801 [closed] related — (+170/-4) — superseded by merged #20892: Adds the per-platform gateway_restart_notification boolean and gates shutdown, restart-originator, and startup/home notifications; it remains relevant as the original implementation whose commits were cherry-picked into #20892.
• #20892 [merged] related — (+171/-4) — merged reference implementation: Lands #20801's coarse per-platform notification opt-out with config round-trip/coercion and coverage across all lifecycle paths, but does not provide a dedicated destination for enabled notifications.
• #31046 related — (+413/-0) — merge: Extends the merged boolean with a single per-platform override destination and routes shutdown session/home, post-restart, and startup notifications there. Consistent with the keep_open review on #31046, the revised diff addresses its blockers by checking SendResult.success with fallback, bridging both config shapes, covering every lifecycle path, preserving colon-bearing Matrix room IDs verbatim, and removing the unrelated SQLite change.
• #52584 [closed] related — (+366/-15) — distinct closed alternative: Adds a boolean that suppresses lifecycle notices in bare channels/groups while retaining DMs and active threads, requiring persisted chat_type data; it reduces noise but does not consolidate notifications into the single ops destination requested by #31046.
• #56015 [closed] related — (+196/-10) — closed alternative, superseded by tracking #52584/#31046: Adds an allowlist that suppresses delivery to existing targets outside selected IDs, but it does not reroute lifecycle events to an ops channel when that channel was not already a delivery target.

Duplicates

#20801 and #20892 implement substantially the same coarse per-platform opt-out; #20892 is the merged salvage of #20801. #31046, #52584, and #56015 overlap in reducing lifecycle-notification noise but have materially different routing semantics.

Suggested consolidation

Merge #31046 — it builds on the already-merged #20892 kill switch and directly solves the remaining single-ops-channel routing need across all lifecycle paths, with the contributor's keep_open review issues addressed in the current diff. #20801 is already superseded by #20892; #52584 and #56015 are already closed and should remain reference alternatives rather than duplicates of #31046.

Cross-PR triage: Reviewed 5 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 85 kB of PR diffs, 16 kB of issue/PR text, 22 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.