Skip to content

fix: kanban notifier ownership - #70091

Closed
duu261 wants to merge 4 commits into
NousResearch:mainfrom
duu261:fix/kanban-profile-notifier-ownership
Closed

fix: kanban notifier ownership#70091
duu261 wants to merge 4 commits into
NousResearch:mainfrom
duu261:fix/kanban-profile-notifier-ownership

Conversation

@duu261

@duu261 duu261 commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Fix Kanban notifier ownership so claims and delivery follow the subscription owner, not the task assignee, with deterministic legacy handling.

Owner / adapter contract

  1. assignee is display-only; it never selects the notifier owner.
  2. Exact notifier_profile rows claim atomically only for that owner.
  3. Standalone gateways serve active-profile rows through self.adapters[platform].
  4. Multiplex gateways serve active-profile rows through self.adapters and secondary owners through the exact _profile_adapters entry for that profile.
  5. Missing secondary adapters fail closed and rewind; no foreign-owner fallback to the active/default bot.
  6. Legacy notifier_profile IS NULL/'' rows are claimable only by active default, through the default adapter.
  7. New auto-subscriptions resolve owner as session profile → HERMES_PROFILE → canonical active profile → default, so new rows are never ownerless.
  8. New rows persist chat_type; legacy missing values keep the documented group wake fallback, and task.session_id remains the synthetic-wake target.

Related work disposition

Tests

  • scripts/run_tests.sh tests/gateway/test_kanban_notifier.py tests/hermes_cli/test_kanban_notify.py tests/hermes_cli/test_kanban_core_functionality.py — 200 passed
  • scripts/run_tests.sh tests/gateway/test_kanban_notifier.py tests/tools/test_kanban_tools.py tests/plugins/test_kanban_dashboard_plugin.py tests/hermes_cli/test_kanban_db_init.py — 255 passed
  • git diff --check passed
  • Ruff on the four amended files passed
  • Added-line secret/privacy scan passed

Compatibility / rollback

Legacy ownerless rows remain default-owned. Roll back by reverting the four commits on this branch.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery comp/cron Cron scheduler and job management comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #57993, #62380, and #67673. This broader patch changes profile-scoped polling, atomic claims, legacy ownerless-row handling, and wake chat-type persistence; please choose the intended ownership/adapter-routing contract before consolidating the competing notifier work.

@duu261
duu261 marked this pull request as ready for review July 23, 2026 13:52
Copilot AI review requested due to automatic review settings July 23, 2026 13:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Route multiplexed notifier rows through their registered profile adapters and keep standalone/default legacy claims scoped to the active owner. Incorporate the active-profile fallback from #57995.

Co-authored-by: tymrtn <ty@tmrtn.com>
@duu261

duu261 commented Jul 23, 2026

Copy link
Copy Markdown
Author

Ownership contract chosen and pushed in 32a9225b4:

  • Assignee is display-only. Exact notifier_profile owns the atomic claim.
  • Standalone active-owner rows use self.adapters; multiplex secondary-owner rows use that exact _profile_adapters entry. Missing entries rewind and never fall back across bots.
  • Only active default claims legacy ownerless rows.
  • Auto-subscribe precedence is session → env → canonical active → default; chat_type is persisted, with legacy group fallback.
  • Polling stays independent from dispatcher ownership/config.

Overlap: #57993/#57995 stamping is incorporated and credited (with #66408 covering the same narrow root); #62380's adapter distinction is compatible without its truncation feature; #67673's decoupling is superseded here without scheduled/truncation scope.

Validation: required slices passed 200/200 and 255/255; git diff --check, Ruff, and added-line secret/privacy scan passed.

@duu261

duu261 commented Jul 23, 2026

Copy link
Copy Markdown
Author

Ownership/adapter contract fixed and credited:

No contributor credit lost.

@falkoro

falkoro commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Scoping this comment honestly up front: this is an interaction/coordination check, not a full logic review of the 744 lines here. I have context on the other half of the kanban notification path, so I tested where the two meet rather than re-deriving your ownership model.

Context. This PR reworks the gateway-side notifier (gateway/kanban_watchers.py, hermes_cli/kanban_db.py) so a subscription is delivered by the profile that owns it. #66435 (the current-main salvage of #60085) adds the other end — delivery of notify subscriptions into TUI/desktop sessions via tui_gateway/server.py. Same feature path, opposite ends, so it was worth checking they compose.

They do. Checked textually first — git merge-tree reports the two merge clean, and the file sets are disjoint (you: gateway/, hermes_cli/, plugins/kanban/, tools/kanban_tools.py; #66435: tui_gateway/server.py only). Then I built the actual combined tree (pr70091 + merge pr66435) and ran both sides' suites on it, Linux / py3.12:

tests/tui_gateway/test_kanban_notify_poller.py   10 passed   # the TUI delivery end
tests/gateway/test_kanban_notifier.py            15 passed   # your ownership routing

Then the whole kanban surface on that same combined tree:

tests/gateway/test_kanban_notifier.py  tests/gateway/test_session_env.py
tests/hermes_cli/test_kanban_core_functionality.py  tests/hermes_cli/test_kanban_db_init.py
tests/hermes_cli/test_kanban_notify.py  tests/plugins/test_kanban_dashboard_plugin.py
tests/tools/test_kanban_tools.py  tests/tui_gateway/test_kanban_notify_poller.py

469 passed in 27.38s

So no merge-order constraint between the two, and the profile-ownership rewrite doesn't disturb the TUI poller's list_notify_subs consumption — worth knowing since your diff changes how list_notify_subs is called (per-profile subs.extend(...) rather than a single flat read).

I have not independently verified the cross-profile mis-delivery this fixes, or reviewed the claim-rewind logic — flagging that so nobody reads this as broader endorsement than it is. The composition result is the part I actually executed.

@teknium1

Copy link
Copy Markdown
Contributor

Superseded — closing with credit. The goals here landed across PR #72191 (chat_type persistence + DM wake, now on main), #72236 (notifier reliability), and #72241 (ownership stamping + profile-correct adapter routing, with per-contributor attribution to the earliest submitters of each half). The remaining delta in this branch — removing the dispatch_in_gateway notifier gate — was deliberately not taken: without replacement scoping it would resurrect the N-gateway SQLite contention the gate exists to prevent. If you want to pursue that, the open design question is tracked as item 1 of #56802 (e.g. a kanban.notify_in_gateway config). Thanks for the comprehensive analysis in this PR — it corroborated several of the fixes that landed.

@teknium1 teknium1 closed this Jul 26, 2026
@duu261
duu261 deleted the fix/kanban-profile-notifier-ownership branch July 31, 2026 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants