Skip to content

fix: auto-subscribe on CLI kanban create + check SendResult in notifier - #45940

Closed
hannes-x wants to merge 3 commits into
NousResearch:mainfrom
hannes-x:fix/auto-subscribe-cli-create
Closed

fix: auto-subscribe on CLI kanban create + check SendResult in notifier#45940
hannes-x wants to merge 3 commits into
NousResearch:mainfrom
hannes-x:fix/auto-subscribe-cli-create

Conversation

@hannes-x

@hannes-x hannes-x commented Jun 14, 2026

Copy link
Copy Markdown

Summary

Two related kanban notification fixes:

1. Auto-subscribe on CLI kanban create

hermes kanban create invoked via CLI did not auto-subscribe the caller. Added auto-subscribe to _cmd_create using get_session_env().

  • Added build_session_subprocess_env() in gateway/session_context.py
  • Replaced inline ContextVar injection in _make_run_env with new helper
  • Added auto-subscribe logic to _cmd_create in hermes_cli/kanban.py

2. Check SendResult.success in notifier

The notifier called adapter.send() but ignored the returned SendResult. Silent failures (thread not found, etc.) were treated as success — the notifier logged 'delivered', removed the subscription, and the user never received notifications.

  • Now checks result.success after adapter.send()
  • Raises RuntimeError on failure, caught by existing except block for proper rewinding

Test Plan

  • hermes kanban create auto-subscribes correctly
  • Subscription target matches current thread
  • Notifier handles SendResult(success=False) by rewinding instead of deleting subscription

hannes-x added 2 commits June 14, 2026 12:41
…on_subprocess_env()

- Added build_session_subprocess_env() in gateway/session_context.py
- Replaced inline ContextVar injection in _make_run_env (local.py) with new helper
- Key fix: old code skipped empty ContextVar values (treated '' same as _UNSET),
  causing fallback to stale os.environ. New helper respects explicit empty values
  and only falls back to os.environ when ContextVar is truly unset.
…elivery failures

adapter.send() returns SendResult(success=False, error=...) for silent
failures (thread not found, channel not found, etc.) without raising
exceptions. The notifier ignored the return value, logged 'delivered',
and removed the subscription — leaving the user with no notification
and no way to recover.

Now we check result.success and raise RuntimeError on failure, which is
caught by the existing except block that handles rewinding and failure
counting correctly.
@hannes-x hannes-x changed the title fix: auto-subscribe on CLI kanban create (bridge ContextVar to _cmd_create) fix: auto-subscribe on CLI kanban create + check SendResult in notifier Jun 14, 2026
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels Jun 14, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification review — reviewed the diff across all 4 files.

gateway/kanban_watchers.py: The SendResult.success check is correct — it converts a silently-failed delivery into a caught RuntimeError, which will be handled by the existing try/except around the send loop. This prevents the notifier from logging "delivered" when the message actually failed.

gateway/session_context.py: build_session_subprocess_env() properly bridges ContextVars to subprocess environments. The behavior change from the original inline code (setting empty-string values when ContextVar is explicitly set to "") is intentional and documented — this correctly distinguishes "never set" from "explicitly empty."

hermes_cli/kanban.py: Auto-subscribe on kanban create follows the same pattern as the slash-command handler. The json mode guard prevents implicit subscription in scripting contexts.

tools/environments/local.py: Clean refactor — delegates to the new shared helper.

All changes look correct. No issues found.

…dResult tests

- Add --subscribe-platform/chat-id/thread-id/user-id to
- Explicit params take priority over env-based auto-subscribe
- Subscription status reported in create output
- Fix all existing notifier tests to return SendResult(success=True)
- Add tests for SendResult(success=False), mixed subs, adapter exception
cwest added a commit to cwest/hermes-agent that referenced this pull request Jun 21, 2026
…bump base-tag

NousResearch#44338 was closed administratively (fork CI gating), not merged, so the
manifest's default "auto-retire when the PR lands in a release" rule can
never fire for this row — and the only strictly-weaker alternative PR
(NousResearch#45940, detection-only) would regress the keep-alive and backoff behaviors
if the carry were dropped on its merge.

Rewrite the NousResearch#44338 row's retire trigger to be behavior-keyed: retire only
when upstream gateway/kanban_watchers.py implements ALL of (i) SendResult
failure-detection, (ii) keep-subscription-alive-on-permanent-failure, and
(iii) bounded exponential backoff. Watch NousResearch#45940 and NousResearch#46443 but do not drop on
NousResearch#45940 merge alone. Update the port-location note to the post-refactor home
(GatewayKanbanWatchersMixin in gateway/kanban_watchers.py) and bump the row's
base-tag to v2026.6.19. Add a "per-row override" caveat to the global
Auto-retire rule so a future rebaser does not naively apply the PR-merge rule
to a behavior-keyed row.
wernerhp pushed a commit to wernerhp/hermes-agent that referenced this pull request Jul 6, 2026
Kanban workers spawned by _default_spawn inherit os.environ but not the
gateway's ContextVar session state (HERMES_SESSION_PLATFORM/CHAT_ID/
THREAD_ID). Workers therefore have no originating thread context, causing
send_message calls to post as new root messages instead of replying in the
correct thread.

Add build_session_subprocess_env() to gateway/session_context.py (mirrors
the approach in upstream PR NousResearch#45940). Overlay the current ContextVar values
onto the base env dict before handing it to Popen. The try/except ImportError
guard keeps the standalone CLI and test environments working unchanged.

Workers now inherit HERMES_SESSION_PLATFORM, HERMES_SESSION_CHAT_ID, and
HERMES_SESSION_THREAD_ID, enabling send_message to auto-target the
originating Mattermost/Telegram/Discord thread without requiring the thread_id
to be baked into the kanban brief.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the silent notifier-failure path. The notifier portion still addresses a real current-main defect: gateway/kanban_watchers.py:416-422 awaits adapter.send() but ignores its SendResult, while the rewind path at :448-476 only handles raised exceptions.

Problems

  • In the added hermes_cli/kanban.py _cmd_create branch, kb.create_task() runs before incomplete explicit subscription arguments return status 2. Invalid one-sided flags therefore create a task but report failure without its ID. Validate the pair before task creation and test that no row is persisted.
  • Do not carry the stale tools/environments/local.py replacement as written. Current main's _inject_session_context_env() (tools/environments/local.py:336-380, cc395e8050) strips unset session variables in engaged concurrent hosts; the PR helper's unset-to-os.environ fallback would defeat that guard if applied mechanically.
  • Current main explicitly treats direct CLI/cron as having no persistent auto-subscription target (tools/kanban_tools.py:990-1032); gateway slash creates already subscribe at gateway/slash_commands.py:450-488.

Suggested changes

  • Salvage the SendResult.success check with a current regression test, validate explicit arguments before creation, and leave the current session-env chokepoint intact.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026
wernerhp pushed a commit to wernerhp/hermes-agent that referenced this pull request Jul 25, 2026
Kanban workers spawned by _default_spawn inherit os.environ but not the
gateway's ContextVar session state (HERMES_SESSION_PLATFORM/CHAT_ID/
THREAD_ID). Workers therefore have no originating thread context, causing
send_message calls to post as new root messages instead of replying in the
correct thread.

Add build_session_subprocess_env() to gateway/session_context.py (mirrors
the approach in upstream PR NousResearch#45940). Overlay the current ContextVar values
onto the base env dict before handing it to Popen. The try/except ImportError
guard keeps the standalone CLI and test environments working unchanged.

Workers now inherit HERMES_SESSION_PLATFORM, HERMES_SESSION_CHAT_ID, and
HERMES_SESSION_THREAD_ID, enabling send_message to auto-target the
originating Mattermost/Telegram/Discord thread without requiring the thread_id
to be baked into the kanban brief.
@teknium1

Copy link
Copy Markdown
Contributor

Closing — both halves are now covered on main: the SendResult(success=False) check landed earlier (rewind-on-soft-fail in the notifier), and CLI auto-subscribe-on-create is tracked by #50972's scope. Your report of the silent notifier-failure path was accurate and corroborated the reliability work that landed via #72236. 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

comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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.

4 participants