feat(kanban): board-wide auto-subscribe via kanban.default_notify - #39
Conversation
Add a config-driven board-wide notify fan-out so every ticket's terminal events (completed/blocked/gave_up/crashed/timed_out + artifacts) report to listed chats with zero manual `notify-subscribe`. - gateway/kanban_watchers.py: `_resolve_default_notify_targets(load_config)` reads `kanban.default_notify` fresh each tick (like `_resolve_auto_decompose_settings`), normalizes/validates, fails safe to []. In `_collect()`, before reading subs per board, INSERT-OR-IGNORE each target onto every active (non-done/archived) task whose platform adapter is connected. Idempotent; never disturbs existing per-task subscriptions. - hermes_cli/config.py: `kanban.default_notify: []` in DEFAULT_CONFIG (empty = off; backward compatible). No new HERMES_* env var. - tests/gateway/test_kanban_default_notify_live.py: 10 resolver cases. Cache/alternation invariants untouched (gateway background loop + config only). Per-task notify path unchanged (no regression). chat_id for "Kanban Workers" proven via signal-cli account.db group_v2 title->id mapping. Patch note: ~/.hermes/plans/hermes-patches/kanban-default-notify.md
There was a problem hiding this comment.
Code Review
This pull request introduces a board-wide auto-subscribe feature (kanban.default_notify) that automatically subscribes configured notification targets to all active tasks on each notifier tick. The review feedback points out a critical performance issue where executing _kb.add_notify_sub in a loop for every active task results in O(N) write transactions per tick, which can cause database write contention. The reviewer suggests optimizing this by using a single bulk INSERT OR IGNORE query per target in a single transaction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if default_notify_targets: | ||
| try: | ||
| active_tasks = _kb.list_tasks(conn) | ||
| except Exception as exc: | ||
| active_tasks = [] | ||
| logger.debug( | ||
| "kanban notifier: cannot list tasks on board %s for default-notify: %s", | ||
| slug, exc, | ||
| ) | ||
| FINAL_STATUSES = {"done", "archived"} | ||
| for t in active_tasks: | ||
| if getattr(t, "status", None) in FINAL_STATUSES: | ||
| continue | ||
| for tgt in default_notify_targets: | ||
| if tgt["platform"] not in active_platforms: | ||
| continue | ||
| try: | ||
| _kb.add_notify_sub( | ||
| conn, | ||
| task_id=t.id, | ||
| platform=tgt["platform"], | ||
| chat_id=tgt["chat_id"], | ||
| thread_id=tgt["thread_id"], | ||
| notifier_profile=notifier_profile, | ||
| ) | ||
| except Exception as exc: | ||
| logger.debug( | ||
| "kanban notifier: default-notify subscribe failed for %s on %s: %s", | ||
| t.id, tgt["platform"], exc, | ||
| ) |
There was a problem hiding this comment.
Executing _kb.add_notify_sub in a loop for every active task on every notifier tick (which runs every 5 seconds) is highly inefficient. Since add_notify_sub opens and commits an IMMEDIATE write transaction for each task, this approach will result in
Instead, we can perform a single bulk INSERT OR IGNORE ... SELECT ... query per target in a single transaction. This reduces the database overhead to
if default_notify_targets:
now = int(time.time())
for tgt in default_notify_targets:
if tgt["platform"] not in active_platforms:
continue
try:
with _kb.write_txn(conn):
conn.execute(
"""
INSERT OR IGNORE INTO kanban_notify_subs (
task_id, platform, chat_id, thread_id, notifier_profile, created_at
)
SELECT id, ?, ?, ?, ?, ?
FROM tasks
WHERE status NOT IN ('done', 'archived')
""",
(
tgt["platform"],
tgt["chat_id"],
tgt["thread_id"],
notifier_profile,
now,
),
)
except Exception as exc:
logger.debug(
"kanban notifier: default-notify bulk subscribe failed for %s on %s: %s",
tgt["chat_id"], tgt["platform"], exc,
)There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c726661b0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if getattr(t, "status", None) in FINAL_STATUSES: | ||
| continue |
There was a problem hiding this comment.
Do not skip fast completions before subscribing
When a task is created and completes before the notifier's next poll (the watcher has an initial 5s delay and then polls on the interval), this final-status guard prevents kanban.default_notify from inserting any subscription row, so the later completed event is never claimed or delivered to the default chat. This drops exactly the short/quick completion notifications the board-wide setting is meant to cover; inserting the default row before excluding already-done tasks (or otherwise processing recent completed tasks) is needed to avoid missing them.
Useful? React with 👍 / 👎.
| if tgt["platform"] not in active_platforms: | ||
| continue | ||
| try: | ||
| _kb.add_notify_sub( |
There was a problem hiding this comment.
Remove stale auto-subscriptions when config targets change
This only ever INSERT OR IGNOREs default-notify rows, so after a target has been present for one tick, deleting it from kanban.default_notify does not stop notifications for any already-subscribed tasks; the persisted rows remain indistinguishable from manual subscriptions and will keep delivering until those tasks are finally done/archived. That contradicts the live-edit behavior described for removing targets and can keep sending noisy or unwanted task updates to a chat after it has been removed from config.
Useful? React with 👍 / 👎.
| if tgt["platform"] not in active_platforms: | ||
| continue | ||
| try: | ||
| _kb.add_notify_sub( |
There was a problem hiding this comment.
Preserve dead-chat suppression for default targets
For a default target whose chat is invalid or whose adapter keeps failing while the task stays non-final (for example blocked after a crash/give-up cycle), the existing send-failure path drops the subscription after MAX_SEND_FAILURES, but the next tick immediately recreates it here because the config still lists the target. That resets the row cursor to 0 and the failure counter was popped when dropping the row, so the same terminal events are retried/replayed forever instead of honoring the dead-chat suppression.
Useful? React with 👍 / 👎.
| kcfg = cfg.get("kanban", {}) if isinstance(cfg, dict) else {} | ||
| raw = kcfg.get("default_notify") or [] |
There was a problem hiding this comment.
Fail closed when the kanban section is malformed
If a live config edit leaves kanban as a non-dict value (for example kanban: false or a YAML list), this .get() raises before _collect() runs, and the outer tick handler skips all existing per-task notification delivery every interval. Since this resolver is meant to fail safe to [], guard that kcfg is a dict so malformed default_notify config cannot break unrelated kanban subscriptions.
Useful? React with 👍 / 👎.
A malformed live edit (kanban: false / a YAML list) made _resolve_default_notify_targets raise on kcfg.get(), breaking the notifier's documented fail-safe-to-[] contract and skipping all per-task delivery every tick. Guard isinstance(kcfg, dict). Addresses gemini P2 (line 84). Patch note: ~/.hermes/plans/hermes-patches/kanban-default-notify.md
… txns The per-task add_notify_sub loop opened an IMMEDIATE write transaction for every active task every 5s notifier tick, even when the rows already existed (INSERT OR IGNORE no-op). On busy boards that is O(tasks*targets) write txns/tick and contends with the dispatcher. Add kanban_db.add_default_notify_subs: one INSERT OR IGNORE ... SELECT per connected target picks all active (non-final) tasks in a single transaction. Idempotent on the PK; existing per-task/default subs and their cursors are never disturbed. Addresses gemini HIGH (line 341). Patch note: ~/.hermes/plans/hermes-patches/kanban-default-notify.md
|
Addressed two review findings; flagging three P2s as deliberate design choices for human call. Fixed
Both covered by new tests + the existing two-tick integration suite (14 passing). Deliberately not changing (design tradeoffs, not bugs)
The last two share one fix (mark default-origin subscriptions) — worth a follow-up if board-wide notify sees heavy use, but it adds schema + reconciliation that this minimal-footprint PR shouldn't carry. |
Goal
Make every Kanban ticket's terminal output (completed / blocked / gave_up / crashed / timed_out, plus completion artifacts) report automatically to one or more chats — e.g. the Signal "Kanban Workers" group — so an operator drives all work from chat and never opens the dashboard. No per-ticket
notify-subscribeby hand.The gap
The notify subsystem (
gateway/kanban_watchers.py::_kanban_notifier_watcher+ thekanban_notify_substable) was entirely per-task. There was no board-wide "subscribe this chat to ALL tickets" knob. This adds one.Design (smallest footprint, survives restart)
New config knob under the live
kanban:section (behavioral config →config.yaml, not a newHERMES_*env var, per AGENTS.md):Empty list (default) = feature off → fully backward compatible.
_resolve_default_notify_targets(load_config)— readskanban.default_notifyfresh each notifier tick (mirrors_resolve_auto_decompose_settings), normalizes/validates, and fails safe to[]on any read/parse error so the notifier loop never dies. Editing the list takes effect next tick — no restart._collect(), before reading the subscription table per board: for every active (non-done/archived) task,add_notify_sub(...)each config-listed target whose platform adapter is connected.add_notify_subisINSERT OR IGNOREon the(task_id, platform, chat_id, thread_id)PK → idempotent, and never disturbs an existing per-task subscription (cursor / fail-count state keyed identically). The rest of the delivery path (claim → send → artifact upload → advance cursor → unsub on done/archived) is unchanged.Why it earns its place
notify-subscribepath untouched — proven by the existing notifier suites still green.Verification
account.dbgroup_v2.group_data:group:y94kMF95...↔ "Kanban Workers" (and/8WZ1w0...↔ "Kanban Master"). The adapter'ssend()doesparams["groupId"] = chat_id[6:], sogroup:<base64>is the outbound id.type: SUCCESS.tests/gateway/test_kanban_default_notify_live.py— 10 resolver cases + 2 integration tests that drive the real notifier mixin against a real temp board: a completed ticket fans out to the configured target with zero manual subscribe (realistic two-tick lifecycle: tick feat(approval): block merges/pushes to main + destructive gh subcommands #1 subscribes active task, task completes, tick fix(stale-call): drop is_local_endpoint inf-timeout bypass #2 delivers), and an existing per-task sub coexists (no regression).Patch note:
~/.hermes/plans/hermes-patches/kanban-default-notify.mdDo not merge — for Eric to review + merge.