feat(messaging): add explicit active handoff primitives - #37865
feat(messaging): add explicit active handoff primitives#37865verybigdog wants to merge 5 commits into
Conversation
0cbd4cc to
6a48465
Compare
|
Marked ready for review. This PR intentionally keeps the primitive small/default-off:
One implementation detail I'm happy to adjust: |
6a48465 to
bd01012
Compare
|
Refreshed this PR onto current Current branch contents are only the generic active handoff primitive:
Intentionally excluded from this upstream PR:
Verification after refresh:
|
bd01012 to
f3c9a64
Compare
f3c9a64 to
9829f4e
Compare
|
Refreshed this PR onto current Changes since the previous head:
Verification on the refreshed branch: New head: |
9829f4e to
3e79286
Compare
|
Refreshed against latest Evidence:
Scope note from review hardening:
|
|
Refreshed again against current Evidence:
|
3e79286 to
e871486
Compare
|
Hi @verybigdog 👋
Let me know what works best for you. If useful, here's the diff of my version: main...nikitaBarkov:nikita.barkov/wake_notify_kanban |
|
Thanks @nikitaBarkov — yes, this is still active, and I agree it needs a refresh against current Your direction looks like the right abstraction to me. In particular:
Since both branches touch the same gateway/Kanban/CLI surface and both are stale/conflicting against current I’ll use this as the target shape for the refresh:
If you strongly prefer this to land through your branch instead, say so and I can adapt, but a clean current-main branch will probably be easier to review. |
e871486 to
1f5101e
Compare
|
Refreshed this PR on current Head: What changed:
Verification run locally from the refreshed worktree:
PR is no longer conflicting: |
|
Follow-up for review BLOCK on gateway auto-subscribe delivery_mode/chat_type: Fixed in new head Changes:
Verification:
Credit/history: this is a narrow follow-up commit on top of the existing PR branch; Nikita’s existing contribution remains intact. |
|
Final fan-in after the gateway auto-subscribe remediation:
Credit/coordination: Nikita Barkov's design/diff remains credited in commit |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying the delivery-mode and persisted-routing design through the notifier path. The current-main premise is real: gateway/kanban_watchers.py:517-548 still hardcodes wake sources to chat_type="group".
Problems
tools/send_message_tool.py:534-535falls back torunner.adaptersafter_authorization_adapter()returnsNone. That reintroduces the cross-profile wrong-bot delivery that current main deliberately fixed inb225b30d0;_authorization_adapterfails closed for unavailable stamped profiles atgateway/authz_mixin.py:50-57.- The routing data is still incomplete. The PR persists
user_id, butbuild_session_keyprefersuser_id_altfor group/channel isolation (gateway/session.py:936-957). A source carrying a distinct alternate identifier can still wake a parallel session.
Suggested changes
- Preserve the profile chokepoint: return a trigger error rather than falling back to the default adapter for an unavailable non-default profile, with a regression test.
- Persist and replay
user_id_altas well, and test equality between the original and wokenbuild_session_keywhen the two identifiers differ.
Automated hermes-sweeper review.
| adapter = authz_adapter(platform, profile or None) | ||
| except Exception: | ||
| adapter = None | ||
| if adapter is None: |
There was a problem hiding this comment.
Blocking: do not fall back to runner.adapters after _authorization_adapter(platform, profile) returns None for a stamped secondary profile. Current main deliberately fails closed here (gateway/authz_mixin.py:50-57; b225b30d0) because this fallback sends via the default profile's bot. Return a trigger error for that case and add the same no-default-fallback regression.
| @@ -1038,7 +1041,9 @@ def _maybe_auto_subscribe(conn: Any, task_id: str) -> bool: | |||
| conn, task_id=task_id, | |||
| platform=platform, chat_id=chat_id, | |||
| thread_id=thread_id, user_id=user_id, | |||
| chat_type=chat_type, | |||
There was a problem hiding this comment.
Blocking: this routing row records user_id but not user_id_alt. build_session_key() prefers user_id_alt for non-DM participant isolation (gateway/session.py:936-957), so the wake can still create a parallel session. Persist and replay the alternate identifier, with a differing-ID session-key regression.
Persist delivery_mode and chat_type on kanban notification subscriptions so terminal events can either send passively, wake the destination agent, or do both without changing default notify behavior. Keep the wake primitive internal to the gateway/notifier path, retain scheduled wake tasks, and add regression coverage for passive notify, wake-only, session routing, and schema exposure. Co-authored-by: Nikita Barkov <nikita.barkov@users.noreply.github.com> Co-authored-by: Joosung Lee <10740155+verybigdog@users.noreply.github.com>
Fail closed when an active wake is stamped for a non-default profile whose live adapter is unavailable, instead of falling back to the default profile adapter. Persist and replay user_id_alt alongside the existing kanban notify routing tuple so woken turns resolve through the same build_session_key as the originating event when user_id and user_id_alt differ.
dafc0e4 to
e5718e6
Compare
|
Refreshed #37865 on current Head: Fixes in this refresh:
Verification:
|
|
Verdict: BLOCK I re-reviewed current head Blocking finding:
Repro probe on this PR checkout: The model-tool auto-subscribe path in Suggested fix:
Checks run from clean worktree
Because the current PR acceptance criteria explicitly require |
|
Remediation for reviewer BLOCK #37865 (comment) pushed at a6ee9a2. What changed:
Verification:
Live PR state after push: mergeable=MERGEABLE, mergeStateStatus=BLOCKED, checks=[] (no checks reported yet). |
|
Keeping this as the canonical active-wake PR for the M28 consolidation. HEAD (a6ee9a2) already resolves the sweeper review: it fails closed for an unavailable non-default stamped profile adapter (no default-bot fallback) and persists+replays user_id_alt so a woken turn keys to the same build_session_key as the originating event. delivery_mode/chat_type/user_id_alt, parent->child notify-sub inheritance (including delivery_mode/chat_type/user_id_alt), DM/group/thread routing, and 'no model-exposed wake / no body-prose inference' all land here. Targeted tests: 281 passed via scripts/run_tests.sh; merges cleanly against current main. The distinct follow-on surface is delivery-boundary failure receipts (#49752); #49751 is duplicate because #37865 already carries the task-graph subscription inheritance. |
|
@teknium1 can u have a look please? :) |
|
@verybigdog, hey, could u please resolve the conflicts? |
SummaryFour PRs address the active-wake and durable Kanban ACK path: #37865 implements opt-in delivery modes with source-faithful wake routing and graph subscription inheritance; #49593 adds receipt/ledger and diagnostics machinery around an overlapping wake implementation; #49749 isolates wake-text sanitization; and #49751 isolates graph subscription inheritance plus origin wiring. Related pull requests
Duplicates#49593 and #49749 substantially duplicate #37865's active-wake mechanism; #49751 duplicates #37865's Kanban graph-subscription inheritance. #49593 also incorporates the narrower surfaces represented by #49749 and #49751. Suggested consolidationAuthor action on #37865: rebase onto current main and resolve the reported conflicts, then retain its source-faithful, profile-fail-closed delivery-mode and graph-inheritance path while addressing any newly exposed integration failures. Keep #49593, #49749, and #49751 closed as superseded/duplicate implementations; salvage only narrow receipt/diagnostic or sanitization pieces in separate follow-ups that preserve the contributor reviews' routing and operator-observation constraints. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup37865 ["PRs duplicating each other"]
P37865["PR #37865 (open)"]
P49749["PR #49749 (closed)"]
P49751["PR #49751 (closed)"]
end
class P37865 open
class P49749 closed
class P49751 closed
class P37865 target
click P37865 "https://github.com/NousResearch/hermes-agent/pull/37865"
click P49749 "https://github.com/NousResearch/hermes-agent/pull/49749"
click P49751 "https://github.com/NousResearch/hermes-agent/pull/49751"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 4 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 279 kB of PR diffs, 5 kB of issue/PR text, 18 kB of discussion (23 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Resolved the conflicts against current The resolution keeps the PR's explicit Validation:
|
…session routing Salvage of #37865 by @verybigdog. Adds delivery_mode (notify / notify+wake / wake) on kanban notify subscriptions, persists chat_type + user_id_alt so a woken turn reconstructs the creator's real session key, inherits the return path to child tasks, and keeps wake out of the model-exposed send_message schema. Original commits were authored under a local placeholder identity (hermes-agent@users.noreply.local); re-attributed to the contributor's public email.
|
Merged via PR #85487 with your work applied onto current main. Your original commits were authored under a local placeholder identity ( We added two follow-up fixes on top: (1) the migration now backfills pre-existing gateway subscriptions to Thanks for the design and the thorough test coverage — the delivery-mode gate and the user_id_alt round-trip were the right call. |
Summary
send_message(trigger_agent=True)active handoff result plumbing while keeping passive sends as the defaultnotify-subscribe --trigger-agentpersistence and gateway notifier supportOrigin/return_to: Discord ... (<#channel_id>)body prose infers a notify subscription for CLI-created cardsSafety
trigger_agent=True/--trigger-agentis explicitly requested or a CLI-created task carries a narrow, machine-readable DiscordOrigin/return_tolinetriggered_agent/trigger_errortrigger_agent=1Origin/return_toparser intentionally recognizes only explicit Discord channel-id shapes such as<#123...>orchat_id=123...; it is not a general natural-language parserTests
python -m pytest tests/hermes_cli/test_kanban_notify.py tests/gateway/test_kanban_notifier.py tests/gateway/test_kanban_notifier_watcher_dispatch_gate.py tests/tools/test_send_message_tool.py -q→ 160 passed, 2 warningsgit diff --checkpython -m compileall gateway/kanban_watchers.py gateway/slash_commands.py hermes_cli/kanban.py hermes_cli/kanban_db.py tools/send_message_tool.py tests/hermes_cli/test_kanban_notify.py -qRefresh notes
origin/main(4829f8d2cat refresh time)--force-with-leasefrom old headf3c9a6453to new head9829f4ee0Local operator policy and AgentFlow supervisor hooks remain intentionally out of scope.