Skip to content

fix(kanban): stop dispatcher terminal config leaking into assignee workers - #66575

Closed
stantheman0128 wants to merge 1 commit into
NousResearch:mainfrom
stantheman0128:fix/66541-kanban-terminal-env-isolation
Closed

fix(kanban): stop dispatcher terminal config leaking into assignee workers#66575
stantheman0128 wants to merge 1 commit into
NousResearch:mainfrom
stantheman0128:fix/66541-kanban-terminal-env-isolation

Conversation

@stantheman0128

Copy link
Copy Markdown
Contributor

What does this PR do?

A Kanban task assigned to a non-default profile could start a worker whose terminal tool used the dispatcher/gateway profile's terminal: configuration instead of the assignee profile's. This PR keeps the profile boundary intact so a worker uses its own profile's terminal isolation settings.

Verified against origin/main d9ee3424: hermes_cli/kanban_db.py::_default_spawn builds the worker env with env = dict(os.environ), which copies the dispatcher's already-exported profile-derived TERMINAL_* values (docker image/volumes/network, container limits, ssh target, backend, sandbox, ...). It switches HERMES_HOME to the assignee profile but never rebuilds those TERMINAL_*. When the child hermes -p <assignee> starts, config.apply_terminal_config_to_env only overrides a var when the assignee config.yaml sets that terminal.* key, so any key the assignee does not set keeps the dispatcher's inherited value. The boundary leaks both ways: an assignee can receive host paths / container options meant only for the dispatcher, and can miss its own approved Docker volumes.

The fix strips the profile-derived TERMINAL_* (the config.TERMINAL_CONFIG_ENV_MAP values) right after the env copy, so the child rebuilds terminal config from the assignee profile via apply_terminal_config_to_env. This mirrors the existing precedent of dropping leaked env vars from spawned workers (for example #63189). The task-specific TERMINAL_CWD / TERMINAL_TIMEOUT that _default_spawn sets are applied afterward and are unaffected (_worker_terminal_timeout_env already handles an absent baseline).

Related Issue

Fixes #66541

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix (profile / container isolation boundary)

Changes Made

  • hermes_cli/kanban_db.py: in _default_spawn, after copying the environment and switching HERMES_HOME, pop the TERMINAL_CONFIG_ENV_MAP env vars so the assignee profile's terminal config is rebuilt in the child instead of inherited from the dispatcher.
  • tests/hermes_cli/test_kanban_boards.py: add test_default_spawn_strips_dispatcher_terminal_profile_env.

How to Test

  1. Set dispatcher terminal env, e.g. TERMINAL_DOCKER_IMAGE, TERMINAL_DOCKER_VOLUMES, TERMINAL_DOCKER_NETWORK, TERMINAL_CONTAINER_MEMORY, TERMINAL_ENV.
  2. Dispatch a Kanban task assigned to a non-default profile.
  3. Before: the worker's terminal env carries the dispatcher's values. After: those are stripped and the worker rebuilds terminal config from its own profile; task-specific TERMINAL_CWD is still pinned.

Real output (Windows 11, pytest 8.4.2):

$ python -m pytest tests/hermes_cli/test_kanban_boards.py -k "default_spawn" -q
2 passed, 55 deselected

$ python -m pytest tests/hermes_cli/test_kanban_boards.py -q
2 failed, 55 passed

The two failures are test_remove_clears_init_cache_for_recreated_db[True/False], a pre-existing Windows-only os.rename PermissionError during board removal that also fails on origin/main with this change stashed (A/B confirmed), unrelated to this diff.

$ python -m pytest tests/cron/test_terminal_cwd_lock.py tests/gateway/test_config_cwd_bridge.py tests/cli/test_cwd_env_respect.py -q
44 passed

$ python scripts/check-windows-footguns.py --diff origin/main
No Windows footguns found (2 file(s) scanned).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(kanban): ...)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I ran the relevant suites (kanban_boards, terminal/cwd bridge, cron cwd lock, cli cwd) and they pass; the full pytest tests/ tree is not run here because it has pre-existing Windows-only failures unrelated to this change (documented above)
  • I've added a test for this fix
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • N/A (no user-facing docs; behavior is internal worker isolation)
  • N/A (no config keys added or changed)
  • N/A (no architecture/workflow change)
  • I've considered cross-platform impact: the strip is dict.pop, platform-neutral; footgun scan clean

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #66541 and the broader worker-environment work in #55600. This PR specifically fixes inherited profile-derived TERMINAL_* configuration rather than credential forwarding.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

Critical

  • None

Assessment

Kanban worker isolation fix: strips dispatcher/gateway profile-derived TERMINAL_* env vars before spawning assignee workers on a different profile. Prevents the dispatcher's docker image/volumes/network/container limits from leaking across profile boundaries.

Looks Good

  • Correctly strips TERMINAL_CONFIG_ENV_MAP.values() from child env
  • Task-specific TERMINAL_CWD / TERMINAL_TIMEOUT still re-applied as intended
  • New regression test test_default_spawn_strips_dispatcher_terminal_profile_env covers the isolation boundary

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused isolation fix. Current main still copies the dispatcher environment in hermes_cli/kanban_db.py:8196 and only switches HERMES_HOME at hermes_cli/kanban_db.py:8209. In the worker, pre-set terminal variables remain authoritative (tools/terminal_tool.py:1328-1342), so dispatcher terminal settings can survive the profile switch.

The change removes the canonical configuration-derived set from hermes_cli.config.TERMINAL_CONFIG_ENV_MAP before _default_spawn reapplies task-specific cwd and timeout values. This preserves the worker-specific overrides at hermes_cli/kanban_db.py:8220-8258 while allowing the assignee profile to provide its own terminal configuration. The existing comment correctly distinguishes this from credential forwarding.

No post-base changes touch either modified file (git log 7498eae3..HEAD -- hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_boards.py), so the patch should be mechanically salvageable.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 18, 2026
…rkers

_default_spawn copied the dispatcher's full environment (env = dict(os.environ))
including its already-exported profile-derived TERMINAL_* values (docker
image/volumes/network, container limits, ssh target, backend, sandbox, ...),
switched HERMES_HOME to the assignee profile, but never rebuilt those TERMINAL_*.
A Kanban task on a non-default profile therefore ran with the dispatcher's
terminal isolation config instead of its own: it could receive host paths and
container options meant only for the dispatcher, or miss its own approved Docker
volumes.

Strip the profile-derived TERMINAL_* (the config.TERMINAL_CONFIG_ENV_MAP keys)
after copying the env so the child hermes -p <assignee> rebuilds them from the
assignee profile via apply_terminal_config_to_env. Task-specific TERMINAL_CWD and
TERMINAL_TIMEOUT are re-applied afterward and are unaffected.

Fixes NousResearch#66541.
@stantheman0128

Copy link
Copy Markdown
Contributor Author

Closing as author to bring our open PRs on hermes-agent back within a healthy throttle (we had 10 open with only sweeper keep_open and no concrete maintainer change requests for days).

Keeping three Windows-focused PRs open for now:

Happy to reopen this one if a maintainer wants it prioritized. Thanks for the patience.

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

Labels

area/config Config system, migrations, profiles comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Kanban workers inherit dispatcher TERMINAL_* settings instead of assignee profile terminal configuration

4 participants