Skip to content

fix(kanban): widen notifier pre-filter to secondary-profile platforms - #56589

Closed
srojk34 wants to merge 1 commit into
NousResearch:mainfrom
srojk34:fix/kanban-notifier-profile-platform-filter
Closed

fix(kanban): widen notifier pre-filter to secondary-profile platforms#56589
srojk34 wants to merge 1 commit into
NousResearch:mainfrom
srojk34:fix/kanban-notifier-profile-platform-filter

Conversation

@srojk34

@srojk34 srojk34 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Follow-up to the just-merged kanban profile-routing salvage (feat(kanban): route notifications via owning profile + wake creator agent #54872 / fix(kanban): route notifications via owning profile + wake creator agent (salvage #54872) #56531 / security(browser): re-check private-network guard after browser_back navigation #56526's sibling review pass), which fixed the delivery-side adapter lookup to route through the shared _authorization_adapter chokepoint (gateway/authz_mixin.py) so a secondary profile's subscription is never silently delivered by the default profile's bot.
  • That fix left an earlier gate untouched: _kanban_notifier_watcher's _collect() pre-filters subscriptions by active_platforms, a set built only from self.adapters (the default/active profile). A subscription owned by a secondary profile on a platform the default profile never connected at all (e.g. profile beta has a live Discord adapter, but the default profile has no Discord adapter) is dropped by this pre-filter before claim_unseen_events_for_sub is ever called.
  • Unlike the already-guarded disconnected-adapter path (which rewinds an already-claimed event so it retries next tick), an unclaimed event here has nothing to rewind — the subscription is skipped every tick, forever, as long as the default profile's adapter set doesn't change. This is a silent, permanent loss of the task's terminal notification and creator-wake, directly contradicting the stated purpose of routing notifications via the owning profile.
  • Fix: widen active_platforms to the union of self.adapters and every platform present across self._profile_adapters.values(). This is intentionally a coarse pre-filter — the precise per-profile chokepoint (_authorization_adapter, which still forbids default-profile fallback) runs unchanged at delivery time a few lines below, with its existing rewind-on-None safety net for any sub that over-admits here but doesn't actually have a live adapter.

Test plan

  • Added test_notifier_claims_platform_only_a_secondary_profile_owns to tests/gateway/test_kanban_notifier.py, reproducing the exact scenario (default profile has telegram only, secondary profile beta has a live discord adapter, sub is owned by beta on discord).
  • Mutation-verified: reverting the kanban_watchers.py change makes the new test fail (assert 0 == 1, adapter never receives the notification); passes with the fix.
  • Ran the full existing tests/gateway/test_kanban_notifier.py suite (8 tests, including test_notifier_owning_profile_adapter_no_default_fallback) — all pass, no regressions.
  • Ran the broader tests/gateway/ -k kanban suite (22 passed, 2 skipped) — no regressions.

_collect()'s active_platforms pre-filter was derived solely from
self.adapters (the default profile), so a subscription owned by a
secondary profile on a platform the default profile never connected
(e.g. beta owns discord, default has no discord adapter at all) was
skipped before claim_unseen_events_for_sub ever ran. Unlike the
disconnected-adapter path, an unclaimed event is never rewound, so this
was a permanent, silent notification/wake loss — directly contradicting
the point of routing notifications via the owning profile
(c696430/b225b30d0). Same cross-profile-adapter-lookup bug class the
delivery-side _authorization_adapter chokepoint already guards against,
one gate earlier. The precise per-profile check still runs unchanged at
delivery time, with its existing rewind-on-None safety net.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 1, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused follow-up. The premise is confirmed on current main: gateway/kanban_watchers.py:201-204 derives active_platforms only from the active/default self.adapters, and :264-270 skips a secondary-profile subscription before claim_unseen_events_for_sub can claim it. The proposed union is at the correct coarse-filter layer.

The delivery invariant remains intact: gateway/kanban_watchers.py:313-335 resolves through _authorization_adapter and rewinds a claim when the owning profile has no matching adapter. The new regression test exercises the missing default-Telegram/secondary-Discord case and asserts delivery through beta's adapter.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #72241 — your commit was cherry-picked onto current main with your authorship preserved. The widened pre-filter stops secondary-profile events being silently lost (claimed, never rewound). Thanks!

@teknium1 teknium1 closed this Jul 26, 2026
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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants