Skip to content

fix(kanban): forward session context vars into worker subprocess env - #57356

Closed
wernerhp wants to merge 7 commits into
NousResearch:mainfrom
wernerhp:fix/kanban-worker-session-env
Closed

fix(kanban): forward session context vars into worker subprocess env#57356
wernerhp wants to merge 7 commits into
NousResearch:mainfrom
wernerhp:fix/kanban-worker-session-env

Conversation

@wernerhp

@wernerhp wernerhp commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Kanban workers spawned by _default_spawn inherit os.environ but not the gateway's ContextVar session state (HERMES_SESSION_PLATFORM, HERMES_SESSION_CHAT_ID, HERMES_SESSION_THREAD_ID). Workers therefore have no originating thread context -- send_message calls land as new root posts instead of replying in the correct thread.

Fix: add build_session_subprocess_env() to gateway/session_context.py and use it in _default_spawn to overlay ContextVar values onto the worker's env before Popen. The existing except ImportError guard keeps the standalone CLI and test environments working unchanged.

Workers now inherit the originating platform/chat/thread, enabling send_message to auto-target the correct thread without requiring the thread_id to be baked into the kanban task brief.

Related Issue

Relates to #45940 (same root cause, complementary fix -- that PR adds build_session_subprocess_env() + explicit --subscribe-* CLI flags; this PR wires the helper into _default_spawn). Also related to #56781 (addresses the notification delivery side of the same dispatch gap).

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/session_context.py: add build_session_subprocess_env(base_env) -- builds a subprocess env dict with ContextVar session values overlaid on top of os.environ (or a provided base).
  • hermes_cli/kanban_db.py: _default_spawn calls build_session_subprocess_env(dict(os.environ)) instead of dict(os.environ) directly; falls back to dict(os.environ) on ImportError for CLI/test safety.

How to Test

  1. Start the gateway on a Mattermost/Telegram session.
  2. Create a kanban task assigned to a profile that calls send_message in its worker.
  3. Before: worker posts to channel root (no thread context).
  4. After: worker posts as a thread reply in the originating conversation thread.

Checklist

Copilot AI review requested due to automatic review settings July 2, 2026 21:22

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.

Pull request overview

This PR fixes kanban worker subprocesses spawned by hermes_cli.kanban_db._default_spawn not inheriting the gateway session ContextVar state (e.g., HERMES_SESSION_PLATFORM, HERMES_SESSION_CHAT_ID, HERMES_SESSION_THREAD_ID), which previously caused worker send_message calls to lose thread targeting and post as new root messages.

Changes:

  • Add build_session_subprocess_env() in gateway/session_context.py to overlay session ContextVar values into a subprocess environment.
  • Update hermes_cli/kanban_db.py::_default_spawn to use build_session_subprocess_env(dict(os.environ)) when available, with a fallback for non-gateway contexts.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
gateway/session_context.py Introduces a helper to construct subprocess envs that include session ContextVar state.
hermes_cli/kanban_db.py Uses the helper during kanban worker spawn to preserve originating platform/chat/thread context.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gateway/session_context.py
Comment thread hermes_cli/kanban_db.py Outdated
Comment thread hermes_cli/kanban_db.py Outdated
@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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P3 Low — cosmetic, nice to have labels Jul 2, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the ContextVar/subprocess boundary. Current main still has the raw-environment gap at hermes_cli/kanban_db.py:8085, so the underlying issue is real.

Problems

  • The stated send_message outcome is not reached by this change. tools/send_message_tool.py:354-369 obtains chat_id and thread_id only from target; when absent, tools/send_message_tool.py:442-461 selects a configured home channel. Neither path reads HERMES_SESSION_CHAT_ID or HERMES_SESSION_THREAD_ID, so forwarding those variables alone cannot make a direct send reply in the originating thread.
  • The new helper iterates all of _VAR_MAP, but gateway/session_context.py:123-139 includes session key/ID/profile and cron auto-delivery variables beyond the requested platform/chat/thread routing context. That broadens the worker's inherited session identity without a demonstrated consumer.

Suggested changes

  • Wire and test the actual origin-target resolution path for worker delivery, preserving explicit targets.
  • Limit the Kanban bridge to the ContextVars required by that path, or add coverage explaining each additional identity/delivery variable.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026
@wernerhp
wernerhp force-pushed the fix/kanban-worker-session-env branch from ce20649 to 98388fd Compare July 15, 2026 22:21
@wernerhp

Copy link
Copy Markdown
Contributor Author

Both review findings are addressed:

  • send_message origin outcome — fixed in 6adc7041d. A worker send_message with no explicit target now resolves to the forwarded session origin (HERMES_SESSION_PLATFORM/CHAT_ID/THREAD_ID) so the reply lands back in the originating thread; an explicit target is parsed earlier and still wins, never overridden. Tests cover origin-fallback, explicit-target-priority, and rejection when neither is present.
  • over-broad ContextVar forwarding — fixed in 6adc7041d, hardened in 98388fd8f. build_session_subprocess_env now forwards only the narrow routing set (platform, chat_id, thread_id) instead of all HERMES_SESSION_* vars; the narrowed set is asserted in the tests.

@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
Hermes Agent and others added 3 commits July 25, 2026 17:07
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.
… forwarding

Addresses reviewer findings on PR NousResearch#57356:
- send_message with no explicit target now falls back to the forwarded
  origin session (HERMES_SESSION_PLATFORM/CHAT_ID/THREAD_ID) instead of
  rejecting outright, without overriding an explicit target.
- build_session_subprocess_env now forwards only the narrow routing set
  (platform, chat_id, thread_id) instead of all HERMES_SESSION_* vars.

Adds regression tests for both: rejection when no target and no origin,
fallback to origin when target omitted, explicit target takes priority,
and the narrowed subprocess-env forwarding set.
… narrow import guard

- build_session_subprocess_env: default base_env to os.environ when caller
  omits it, and stop the per-var os.environ fallback from overriding an
  explicitly-supplied base_env value (explicit base_env is authoritative).
- _default_spawn: narrow the ImportError guard around the
  gateway.session_context import to ModuleNotFoundError so a real
  regression inside the module propagates instead of being silently
  swallowed.
- Add regression tests pinning both fixes.
@wernerhp
wernerhp force-pushed the fix/kanban-worker-session-env branch from af475b0 to 502d15c Compare July 25, 2026 15:12
Coder added 4 commits July 26, 2026 04:02
…sion-env

Semantic conflict in hermes_cli/kanban_db.py _default_spawn():
upstream main 148497f (salvaged from NousResearch#69181) STRIPS every _VAR_MAP
session-routing key from the dispatched worker env, directly superseding
this PR's approach of FORWARDING HERMES_SESSION_* via
build_session_subprocess_env. A detached worker that inherits routing
auto-subscribes child tasks to an unrelated chat, so the strip is the
shipped, correct behavior. Resolved by taking main's strip; the PR's
_SUBPROCESS_FORWARD_VARS is a subset of _VAR_MAP so nothing new leaks.
send_message_tool.py origin-fallback and build_session_subprocess_env
remain (merged clean) — harmless when the env vars are absent.
# Conflicts:
#	tests/gateway/test_session_env.py
#	tests/tools/test_send_message_tool.py
@wernerhp

wernerhp commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by main.

The underlying concern (a detached kanban worker inheriting a previous gateway turn's send_message routing) is now handled on main in the opposite direction. Rather than forwarding the originating session's routing ContextVars into the worker subprocess, the dispatcher strips them:

# hermes_cli/kanban_db.py, worker spawn env
from gateway.session_context import _VAR_MAP
for key in _VAR_MAP:
    env.pop(key, None)

This PR's build_session_subprocess_env does the reverse (inherit rather than isolate), which would reintroduce the cross-session leak main now guards against. The two are mutually exclusive, so rebasing forward is not the right move.

If per-task routing forwarding is still wanted, an explicit opt-in on the task, layered on top of the default isolation, would be the safer shape than a blanket inherit at spawn.

@wernerhp wernerhp closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

4 participants