fix(kanban): drop approval context env vars from spawned workers - #63189
fix(kanban): drop approval context env vars from spawned workers#63189liuhao1024 wants to merge 1 commit into
Conversation
|
Thanks for jumping on this so quickly — the diagnosis around process-local approval callbacks is right, and scrubbing ambient gateway/cron/session/YOLO state is definitely an important part of the fix. I walked the patch through the current approval flow, though, and I think it needs one more piece before it is safe to merge as a fix for #63183. The main concern is that removing these markers puts the worker into the existing non-interactive, non-gateway, non-ask, non-cron branch. In That is the inverse gap already tracked in #55945, with related proposed work in #55946. It is also why the issue description calls out that environment scrubbing should not ship by itself. A couple of related details:
My suggestion would be to keep the scrub, but pair it with an explicit Kanban/card-owner execution context and policy. If an action requires consent and no durable route exists, the interim behavior should fail closed rather than fall through to generic headless auto-approval. If If the intended scope here is only the environment-cleanup portion, it may be better to present it as a partial mitigation and avoid |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the ambient-environment inheritance path; current main does copy the parent environment into the detached worker (hermes_cli/kanban_db.py:8085, passed at :8229), while the gateway sets HERMES_EXEC_ASK=1 at gateway/run.py:1739.
Problems
- The scrub is incomplete: the changed code removes
HERMES_SESSION_*but notHERMES_GATEWAY_SESSION(hermes_cli/kanban_db.py:7808-7810).tools/approval.py:241-245reads that direct marker, so an inherited legacy marker still places the worker in gateway approval handling. - More importantly, removing ask/gateway context alone changes a recoverable consent path into headless approval.
check_all_command_guards()returnsapproved=Truebefore normal dangerous-command/Tirith processing in that branch (tools/approval.py:2696-2762);check_execute_code_guard()has the same headless approval path (tools/approval.py:3140-3146). - The one-file diff adds no regression test, although
_default_spawnenv capture already has a test seam attests/hermes_cli/test_kanban_core_functionality.py:2785-2815.
Suggested changes
- Pair any scrub with explicit task-scoped Kanban approval policy that fails closed when no durable approval route exists, and test the resulting terminal and
execute_codedecisions.
Automated hermes-sweeper review.
| # approval-capable and attempt to send approval requests that cannot be | ||
| # delivered, causing silent failures or misrouted notifications. | ||
| # See issue #63183. | ||
| env.pop("HERMES_EXEC_ASK", None) |
There was a problem hiding this comment.
Removing the ask marker together with the remaining context markers sends non-hardline worker actions through the documented headless auto-approve path: check_all_command_guards() returns approved before normal dangerous-command/Tirith handling (tools/approval.py:2696-2762), and check_execute_code_guard() returns approved without gateway/ask context (:3140-3146). This needs an explicit Kanban fail-closed policy or durable approval route, not scrub-only behavior.
| env.pop("HERMES_CRON_SESSION", None) | ||
| env.pop("HERMES_INTERACTIVE", None) | ||
| env.pop("HERMES_YOLO_MODE", None) | ||
| for key in list(env.keys()): |
There was a problem hiding this comment.
This prefix cleanup does not cover HERMES_GATEWAY_SESSION, which _is_gateway_approval_context() reads directly (tools/approval.py:241-245). A worker inheriting that legacy marker remains in gateway approval context and can still produce child-local pending approval.
What does this PR do?
Kanban dispatcher spawns detached worker subprocesses via
subprocess.Popen(env=dict(os.environ)), which copies the parent gateway's full environment. This includes approval-related environment variables (HERMES_EXEC_ASK,HERMES_CRON_SESSION,HERMES_SESSION_*,HERMES_INTERACTIVE,HERMES_YOLO_MODE). However, gateway approval notification callbacks and queues are process-local Python dictionaries that cannot be inherited by subprocesses. The detached worker therefore identifies itself as gateway/approval-capable and attempts to send approval requests that cannot be delivered, causing silent failures or misrouted notifications.This fix drops all approval-related environment variables from the worker's environment before spawning, matching the existing pattern for
HERMES_TUI. Workers run in non-approval mode by design—they should never require gateway approval.Related Issue
Fixes #63183
Type of Change
Changes Made
hermes_cli/kanban_db.py: In_default_spawn, add code afterenv.pop("HERMES_TUI", None)to drop approval context environment variables (HERMES_EXEC_ASK,HERMES_CRON_SESSION,HERMES_INTERACTIVE,HERMES_YOLO_MODE, and allHERMES_SESSION_*prefixed keys).How to Test
hermes gateway run --replace).ps eww -p <pid>(on macOS/Linux) or Process Explorer (on Windows).HERMES_EXEC_ASK,HERMES_CRON_SESSION,HERMES_INTERACTIVE,HERMES_YOLO_MODE, and allHERMES_SESSION_*variables are NOT present in the worker's environment.HERMES_KANBAN_TASKvariable should still be present (kanban context is intentional).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A