Skip to content

fix: stop child hermes chat inheriting Kanban worker ownership - #92416

Open
ming0627 wants to merge 2 commits into
NousResearch:mainfrom
ming0627:fix/kanban-child-chat-env-leak
Open

ming0627 wants to merge 2 commits into
NousResearch:mainfrom
ming0627:fix/kanban-child-chat-env-leak

Conversation

@ming0627

Copy link
Copy Markdown

Bug Description

A dispatcher-owned Kanban worker that shells out to hermes chat (for example hermes chat -Q … --source tool for Browser Use) inherited HERMES_KANBAN_TASK / RUN_ID / WORKSPACE / CLAIM_LOCK. The child process is a new interpreter, so in-process ContextVars cannot help: is_dispatcher_owned_worker_context() defaults True, and the child can kanban_complete the parent card, which then deletes the scratch workspace while the real worker is still running.

Fixes #70809

Related (open, not landed, different or incomplete approaches): #70898, #81843.

Root Cause

  1. Terminal / subprocess env builders copied os.environ and only fully scrubbed Kanban identity inside delegate_task / in-process cron contexts.
  2. A nested hermes chat is a new process, so ContextVar isolation from delegate_task children inherit HERMES_KANBAN_TASK and can complete/block the parent's kanban card #56647 / fix(kanban): isolate delegate_task children from the parent's Kanban task #69837 / cron isolation does not apply.

Fix

Defense in depth, transplanted from locally tested commit 2abc0a841 onto current origin/main:

  • Spawn path: _make_run_env, _sanitize_subprocess_env, and hermes_subprocess_env drop lifecycle ownership keys (TASK, RUN_ID, WORKSPACE, WORKSPACES_ROOT, CLAIM_LOCK). Board routing pins (BOARD / DB) stay so hermes kanban shell-outs remain on the same board ([Bug]: harness kanban CLI invoked from agent session ignores active-board pin, races current file with concurrent boards switch #20074). build_subprocess_env(scrub_secrets=True) inherits this via _sanitize_subprocess_env.
  • Startup path: cmd_chat drops the same keys unless this process is the explicit dispatcher worker (HERMES_SESSION_SOURCE=kanban AND query work kanban task <id>). --query-file is checked after the prompt is loaded.
  • Dispatcher _default_spawn still injects ownership into the real worker.

Authorship of the functional commit is preserved (Sherman Lye). A follow-up mapping commit attributes the local author email to ming0627 for contributor-check CI.

How to Verify

  1. Run the targeted isolation suite (see Test Plan).
  2. Confirm a child env from LocalEnvironment.execute / _make_run_env has no HERMES_KANBAN_TASK while HERMES_KANBAN_BOARD / provider-style keys remain.
  3. Confirm cmd_chat --source tool drops inherited ownership, a mismatched work kanban task prompt drops it, and an explicit board-worker launch keeps it.
  4. Confirm kanban_complete from that child env errors and the parent task stays running with its workspace intact.

Test Plan

  • Added regression tests for spawn-path isolation and cmd_chat startup
  • Existing sibling isolation tests still pass
  • Exact incident test: test_terminal_child_cannot_complete_parent_and_parent_stays_running

Verified on this PR branch (fresh clone of current origin/main + transplant), not the live checkout:

scripts/run_tests.sh \
  tests/tools/test_kanban_child_chat_env_isolation.py \
  tests/hermes_cli/test_kanban_child_chat_startup.py \
  tests/tools/test_hermes_subprocess_env.py \
  tests/tools/test_delegate_kanban_isolation.py \
  tests/cron/test_cron_kanban_env_isolation.py \
  tests/tools/test_kanban_tools.py \
  tests/hermes_cli/test_pin_kanban_board_env.py \
  tests/hermes_cli/test_kanban_worker_session_source.py \
  tests/hermes_cli/test_kanban_worker_spawn_toolsets.py \
  tests/tools/test_build_subprocess_env.py \
  tests/agent/test_subprocess_env_guard.py

Result: 11 files, 100 tests passed, 0 failed.

Risk Assessment

Low / Medium — blast radius is env inheritance for subprocesses launched from a Kanban worker and cmd_chat startup. Board pins and Browser Use / provider-key passthrough (inherit_credentials, _BROWSER_PASSTHROUGH_KEYS) are unchanged. Dispatcher workers keep ownership via the explicit argv/env contract.

This is not claiming a local-only commit as landed upstream. Merge to NousResearch/hermes-agent main is the durable landing.

Sherman Lye and others added 2 commits August 23, 2026 01:34
Nested hermes chat --source tool from a Kanban worker inherited
HERMES_KANBAN_TASK and could kanban_complete the parent card. Strip
lifecycle ownership from subprocess env and drop it at chat startup
unless the process is the dispatcher worker.
Map shermanlye@Shermans-MacBook-Pro.local to ming0627 so the
transplanted authorship commit passes contributor attribution CI.
@ming0627

Copy link
Copy Markdown
Author

Local verification on a fresh clone of current origin/main (4b860d819) plus this transplant (not the live install checkout):

scripts/run_tests.sh on the 11 isolation/sibling files: 11 files, 100 tests passed, 0 failed.

GitHub Actions on this fork PR is sitting in action_required (CI / Docker / Nix) with empty job lists — needs a maintainer to approve the workflows. I cannot enable auto-merge (EnablePullRequestAutoMerge denied) and origin is not writable from ming0627.

This is the same class as #70809. Distinct from still-open #70898 (ContextVar gating, failed CI) and #81843 (spawn-path strip only).

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management backend/local Local shell execution P3 Low — cosmetic, nice to have sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 22, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Reviewed agent/delegation_context.py, hermes_cli/main.py (+565 across 6 files) and the new test files.

What's good

  • Genuine defense in depth: spawn-time env scrubbing (scrub_kanban_lifecycle_ownership) and an independent startup drop inside cmd_chat, so the incident survives either layer failing. The lifecycle-vs-routing split is well reasoned and well documented — children keep HERMES_KANBAN_BOARD/DB pins (hermes kanban shell-outs stay on-board, [Bug]: harness kanban CLI invoked from agent session ignores active-board pin, races current file with concurrent boards switch #20074) while losing only the run identity that makes kanban_complete default to the parent card.
  • The --query-file subtlety is handled with unusual care: the first ownership check is deferred when the prompt arrives from disk, then re-run after load, with a comment explaining why a board worker using a query file must not look like an interactive child.
  • The contributors/emails/ mapping preserving the transplant author's identity follows the repo's contributor-credit convention. Tests assert both the drops and the preserved routing pins, plus the negative direction (inherited source=kanban alone doesn't confer ownership).

Suggestions

  1. Contract brittleness: is_explicit_board_worker_launch() authenticates "this is the real board worker" by exact-matching the human-language query "work kanban task <id>". If _default_spawn's prompt wording ever changes (or a profile customizes it), every worker silently loses ownership and can't complete its own card — tasks stall until stale-claim reclaim. Prefer an explicit machine signal from the spawner (e.g. HERMES_KANBAN_WORKER_LAUNCH=<task> written only by _default_spawn, checked alongside source == "kanban"), keeping the query match as a fallback; or at minimum pin a cross-reference test asserting _default_spawn's argv satisfies the matcher so the two can't drift silently.
  2. Grandchild semantics worth one docstring sentence: a worker spawning another hermes chat (no kanban source) now gets ownership dropped — correct — but a worker running hermes chat -q "work kanban task <same-id>" manually would re-acquire ownership of someone else's card if the IDs collide across boards; the DB pin bounds this to the same board, which is probably acceptable — just record the reasoning.
  3. Nit: import os inside drop_inherited_kanban_lifecycle_if_not_board_worker — module-level import matches the rest of the file.

Strong security fix for a real destructive race (child completing the parent card deletes the live workspace); #1 is the one thing I'd harden before merge.

Copy link
Copy Markdown
Contributor

Bound to the expanded ambient process-authority class under #83565 and the governing control-plane architecture in #91911.

This PR supplies the deny-side reference edge: a dispatcher-owned Kanban worker may own its task lifecycle, but an arbitrary nested hermes chat must not inherit that ownership merely because it starts from the worker's process environment.

Keep the board-routing pins and explicit real-worker launch contract; strip lifecycle mutation authority at the child boundary and retain the startup-side proof so a polluted environment cannot re-mint ownership. This is the complement of #92309, which carries bounded authority across one explicitly authorized worker → hermes-tools continuation edge.

Architecture extension: #91911 (comment)
Delivery ledger: #83565 (comment)

@benperry6

Copy link
Copy Markdown
Contributor

Independent follow-up completed for the explicit native-worker launch contract.

PR against this branch: ming0627#1

The follow-up removes human query text from authority and uses a fresh one-shot nonce in both the native spawn env and hidden CLI argument. Startup requires exact source/task/nonce agreement, consumes the env nonce immediately, rejects reconstruction from the public task ID, and fails closed on validator import failure while retaining board/DB routing.

Evidence: 39/39 final relevant tests, repository-wide ownership-key scan 7/7 covered, Ruff/compileall/diff-check clean. Parent is exact head 7fabbc59d9a50c1d12be5bfd4ff9a633db64e734.

Scope note: this prevents accidental/passive lifecycle inheritance; it is not claimed as cryptographic isolation from a malicious same-user process.

@benperry6

Copy link
Copy Markdown
Contributor

Current-main refresh of the machine launch contract requested in review, kept separate from every other Kanban fix.

The issue still reproduced on that base. The refreshed patch removes human query text from authority: the native dispatcher creates a fresh one-shot nonce, passes it through both the worker environment and a hidden CLI argument, requires exact source/task/nonce agreement, and consumes the environment proof at startup. Ordinary child chats retain only board/DB routing pins and lose lifecycle mutation authority.

Verification on the exact commit: the two regression files failed with 21 tests before the source patch; afterward those 21 passed. The 11-file Kanban startup/environment matrix passed with 112 passed and 0 failed. Ruff, compileall, and git diff --check passed.

This supersedes my older author-branch follow-up 507539d6b0ee36e9e1cfca5a1c9ed9dfc0fbe36e. I did not open a competing upstream PR; the commit is available for the existing PR author or maintainers to cherry-pick.

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

Labels

backend/local Local shell execution comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have 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.

[Bug]: nested hermes chat subprocess inherits parent Kanban worker lifecycle ownership

5 participants