Reviewed this end-to-end — really want this landed because the missing-completion-notification gap is biting our team daily. Rebased onto current main (clean, 1/1 commits) and ran the new test files: 57/57 pass in 7.7s. Rebased branch: TheoLong:review/pr-21523-rebase if it's useful as a starting point.

Overall LGTM with one blocker plus a few smaller concerns. Would love to see this merged.

🔴 Blocker

INSERT OR IGNORE in add_notify_sub silently swallows notification_mode updates on duplicate (task_id, platform, chat_id, thread_id) keys.

Scenario: user runs /kanban subscribe first (registers direct), then an agent calls kanban_create for the same task with notification_mode='synthesize'. Second insert is ignored, mode stays direct, the synthesis path the agent requested never runs — and there's no error to debug from.

Suggested fix:

conn.execute(
    """INSERT INTO kanban_notify_subs
       (task_id, platform, chat_id, thread_id, user_id, notification_mode, origin_context)
       VALUES (?, ?, ?, ?, ?, ?, ?)
       ON CONFLICT(task_id, platform, chat_id, thread_id) DO UPDATE SET
         notification_mode = excluded.notification_mode,
         origin_context    = COALESCE(excluded.origin_context, origin_context),
         user_id           = COALESCE(excluded.user_id, user_id)""",
    (...),
)

(Preserves origin_context/user_id if a later call passes nulls; always honors the latest notification_mode.)

🟡 Concerns

1. _synthesize_kanban_notification can block the watcher thread up to 45s with no concurrency cap. Under a burst of completing tasks, direct-mode notifications get starved behind synthesis calls. Worth either (a) running synthesis in a bounded thread pool, or (b) dispatching direct-mode sends first and queuing synthesize-mode separately.

2. _current_user_message_for_tools only retains the most recent message. In multi-turn sessions where the user kicks off kanban work several turns before the tool actually fires, origin_context ends up pointing at unrelated chatter. Could capture at the turn the tool is invoked instead of "latest seen."

3. notification_mode is exposed in the tool schema with silent as a valid value. A model could set silent on a user-visible task and the user would get nothing — no audit, no alert. Recommend either dropping silent from the schema enum (keep it CLI-only) or requiring explicit user opt-in via session config.

✅ LGTM

• SendResult failure → retry wiring is correct
• Schema migration is safe (PRAGMA table_info guard before each ALTER)
• HERMES_KANBAN_TASK env gate correctly prevents worker fan-out from spamming origin chats
• synthesize falls back to direct cleanly on synthesis exception
• _kanban_notify_in_gateway_enabled ownership guard handles multi-profile deployments
• Test coverage solid (487 new lines across two files)

@verkyyi thanks for putting this together. The blocker is a one-line conflict-clause swap; happy to push a fixup commit if it'd help unblock the merge. cc: maintainers — this is a real production issue for agent-orchestrated kanban workflows.

Pushed a one-commit fixup against this branch (rebased on main) addressing the BLOCKER from my earlier review:

Branch: TheoLong:review/pr-21523-rebase → commit 4291acf

Change: add_notify_sub switches from INSERT OR IGNORE to INSERT … ON CONFLICT(task_id, platform, chat_id, thread_id) DO UPDATE SET notification_mode=excluded.notification_mode, … with COALESCE on user_id / origin_session_id / origin_profile / origin_context so a partial re-subscribe (e.g. tool-driven synthesize arriving after slash-command direct) updates the mode without clearing the originating context fields.

Test: tests/tools/test_kanban_tools.py::test_add_notify_sub_upserts_notification_mode_on_duplicate — registers direct first, then synthesize with None for origin fields; asserts the mode flips and the original user_id / origin_* survive. Full suite: 58/58 passing (the existing 57 + the new regression).

Happy to open this as a PR against your branch if that's easier than a cherry-pick — let me know.

— Tem

Thanks Theo — good catch on the duplicate subscription path.

I pulled in the important behavior from your rebase branch: duplicate notify subscriptions now use an upsert instead of INSERT OR IGNORE, so later notification_mode, user_id, and origin/context updates are not silently dropped. I also added regression coverage for that case.

I also addressed the concurrency concern from my local patches in this PR branch: synthesis/native turns for the same origin session now serialize on the active origin session lock, while notifications for unrelated chats do not block behind that lock. Added tests for both the same-origin waiting case and the unrelated-chat non-blocking case.

Pushed the fix in 58b062cd.

I kept this focused to the blocker + concurrency issue. I did not add a separate bounded synthesis worker queue/thread-pool policy here; happy to do that as a follow-up if you think we should make that explicit in this PR.

Thanks @verkyyi — closing this one. Adding notification_mode + origin provenance + a synthesis path across gateway/tools/run_agent is substantive agent-orchestration design. We have the gateway-create auto-subscribe path (origin chat gets terminal-event notifications), plus the kanban-tool send_message bridge for cross-session delivery. A third synthesis path would need to specify what it does that those two don't and how all three avoid drift. Appreciate the work.