Skip to content

fix(kanban): dedupe kanban notifier delivery claims - #22558

Closed
quocanh261997 wants to merge 2 commits into
NousResearch:mainfrom
quocanh261997:kanban/t_4f039931-notifier-fixes
Closed

fix(kanban): dedupe kanban notifier delivery claims#22558
quocanh261997 wants to merge 2 commits into
NousResearch:mainfrom
quocanh261997:kanban/t_4f039931-notifier-fixes

Conversation

@quocanh261997

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes Kanban notification delivery races that can cause duplicate or misattributed completion/block notifications in multi-profile gateway setups.

The issue is that multiple active profile gateways can watch the same Kanban database path, sometimes through different board slugs that resolve to the same SQLite file. Without a single delivery claim, more than one gateway can observe and send the same notification event, and the message may appear to come from a profile that was not part of the original conversation.

This PR makes notification delivery safer by deduplicating board database paths per watcher tick and adding an atomic event-claim flow before delivery. If delivery fails or the platform is disconnected, the notifier rewinds so another attempt can handle the event later instead of silently losing it.

It also hardens Kanban test isolation so tests do not inherit production home-channel or Kanban configuration and accidentally write fake cards/subscriptions into a real user board.

Related Issue

No public issue filed. Follow-up from investigation of Kanban notification duplication/misattribution in a multi-profile Discord gateway setup.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/run.py
    • Deduplicates watched Kanban board database paths per tick.
    • Claims notification events atomically before delivery to avoid duplicate sends across gateway watchers.
    • Rewinds notification cursor/claim state when delivery fails or a platform is disconnected.
    • Improves notifier logging around delivery, skipped platforms, and failure cases.
  • hermes_cli/kanban_db.py
    • Adds notification event claim/rewind helpers used by the gateway watcher.
  • tests/conftest.py
    • Clears Kanban/home-channel related environment and config pins for safer test isolation.
  • tests/gateway/test_kanban_notifier.py
    • Adds coverage for duplicate board path handling, atomic claims, disconnected platform behavior, and delivery failure rewind.
  • tests/hermes_cli/test_kanban_core_functionality.py
    • Updates Kanban core tests for the notification claim/delivery behavior.

How to Test

Reproduction before this fix:

  1. Run multiple Hermes profile gateways that watch the same Kanban database, for example several Discord profile gateways using the same default board.
  2. Subscribe a thread/channel to a Kanban task.
  3. Complete or block the task.
  4. Observe that more than one gateway can race to send the same lifecycle notification, and the notification may appear under a profile that did not own the conversation.

Verification for this branch:

  1. Run the focused notifier and Kanban tests with an isolated Hermes home:

    HERMES_HOME=$(mktemp -d) HOME=/home/mike python -m pytest \
      tests/gateway/test_kanban_notifier.py \
      tests/hermes_cli/test_kanban_core_functionality.py \
      -q
  2. Confirm the result:

    154 passed in 3.00s
    
  3. Carl also ran these focused checks before handoff:

    HERMES_HOME=$(mktemp -d) python -m pytest tests/gateway/test_kanban_notifier.py -q
    HERMES_HOME=$(mktemp -d) python -m pytest tests/hermes_cli/test_kanban_core_functionality.py -q

Platforms tested:

  • Ubuntu 24.04 / Linux 6.8 on the Hermes production host.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu 24.04 / Linux 6.8

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Focused test output from this branch:

Using isolated HERMES_HOME=/tmp/tmp.DURYFFXCQX
bringing up nodes...
bringing up nodes...

........................................................................ [ 46%]
........................................................................ [ 93%]
..........                                                               [100%]
154 passed in 3.00s

@quocanh261997
quocanh261997 force-pushed the kanban/t_4f039931-notifier-fixes branch from 19e0485 to 9e694e5 Compare May 9, 2026 13:07
@quocanh261997 quocanh261997 changed the title fix: dedupe kanban notifier delivery claims fix(kanban): dedupe kanban notifier delivery claims May 9, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Salvage merged via PR #23401 (rebase) — both your commits (the notifier claim/rewind work and the lint.yml fork-PR fix) shipped on main with your authorship preserved. AUTHOR_MAP entry already existed.

Two improvements added during salvage:

  1. Extra regression test (test_kanban_notifier_rewinds_claim_on_send_exception) covering the rewind path when adapter.send() raises mid-tick. Your existing tests cover the adapter-disconnected path; this pins the send-exception path which fires through the same _kanban_rewind site but a different conditional branch.
  2. Dropped the per-delivery success log from INFO to DEBUG. Failure paths stay at WARNING. A busy multi-platform gateway can fire hundreds of these a day; they were drowning real warnings in gateway.log.

The fix interlocks cleanly with PR #22994 (drop redundant init_db() in the same notifier loop) we merged earlier today — both make the loop more correct under multi-process workloads, and the cherry-pick auto-merged with no conflicts.

Thanks @quocanh261997 — well-engineered fix with the right SQLite primitives (BEGIN IMMEDIATE for the claim, CAS on the rewind), and the test coverage of the cross-process race was tight.
#23401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants