Skip to content

fix(skills): make env_passthrough registrations visible across tool context snapshots [risk 0.30] - #131908

Merged
OutThisLife merged 2 commits into
mainfrom
bb/90004-land-90011
Oct 5, 2026
Merged

OutThisLife merged 2 commits into
mainfrom
bb/90004-land-90011

Conversation

@OutThisLife

@OutThisLife OutThisLife commented Oct 3, 2026 •

Copy link
Copy Markdown

Lands #90011 by @liuhao1024 on current main (their commit; conflicts resolved in tools/env_passthrough.py).

The skill env-passthrough allowlist was a session-scoped ContextVar, but tool dispatch fans each tool call onto a worker whose context is a copy_context() snapshot taken at submit time (tools.thread_context.propagate_context_to_thread). A registration made inside skill_view's worker never reached the submitting thread's context, so the very next execute_code/terminal call re-snapshotted the original (empty) allowlist and the skill's declared env vars (prerequisites.env_vars / required_environment_variables) never passed through — the bundled notion/airtable/gif-search/teams skills failed out of the box (#90004). The allowlist is now a process-wide module-level set (matching the config-based allowlist, which was already a module global). Cross-session exposure is limited to the NAMES: values still resolve per profile through resolve_passthrough_value's secret scope, and the provider-credential blocklist (GHSA-rhgp-j443-p4rf) still applies at registration.

Conflict notes

  • main had lightly reworded the ContextVar comment; took the PR's replacement comment block verbatim (it carries the full why).
  • clear_env_passthrough (called by gateway session reset) keeps working — it clears the same set.

Tests

  • tests/tools/test_env_passthrough.py — 30 passed (incl. the two new cross-snapshot visibility tests)
  • tests/tools/test_skill_env_passthrough.py, tests/tools/test_skills_tool.py, tests/tools/environments — passed

Risk — low-medium: changes process-wide visibility of a security-adjacent allowlist, but only the NAMES cross sessions; values remain profile-scoped and credential-scrubbing is unchanged.

Lint: check-windows-footguns, check_bash_shebangs, check_no_tmp_literals (pre-existing main-only flag), check_config_yaml_writers, check_os_marker_fakes, ruff on changed files — all pass.

Side effects

  1. Existing state: none — the allowlist was process-memory only (ContextVar → module-level set); nothing is persisted under HERMES_HOME, no config key or store shape changes. The test-fixture fix commit touches tests/tools/_child_env_fixtures.py only.
  2. Version skew: none — tools/env_passthrough.py is core-process code; no client↔server, desktop↔remote-backend or plugin↔host contract changes. Plugins/skills call the same register_env_passthrough API unchanged.
  3. Reserved values/invariants: none — no config default gains meaning; no existing assertion rewritten (the fixture change adapts isolation mechanics to the new set, preserving each test's contract: start-empty allowlist per test, restored after).
  4. Other callers: every caller of _get_allowed/clear_env_passthrough was grepped — gateway session reset (clear), skill_view registration, execute_code/terminal/build_subprocess_env consumers. The full failing CI surface (test_build_subprocess_env, test_code_execution_modes, test_execute_code_approval_cluster, test_local_env_blocklist, test_env_passthrough, test_skill_env_passthrough) re-ran green locally (all lanes fail on the same fixture AttributeError, fixed by this commit).
  5. Open findings: no reviewer/contributor comments with repros open on the PR; no unanswered findings.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

૮ >ﻌ< ა ci review

ran on acf6f9d — test(child-env): fixture isolates the process-wide skill pas

⚠️ Warnings

CI timings · View report · View job

Wall time 7m28s vs 1m36s (+366.7%). 2 faster,

  • Detect affected areas: -65.0s
  • All required checks pass: -3.0s

liuhao1024 and others added 2 commits October 2, 2026 20:26
…ontext snapshots

skill_view registers a skill's declared env vars (required_environment_
variables and the legacy prerequisites.env_vars) into the sandbox
passthrough allowlist. The allowlist was backed by a ContextVar, and
tool dispatch fans each tool call onto a worker whose context is a
copy_context() snapshot taken at submit time
(tools.thread_context.propagate_context_to_thread). The registration
therefore lived and died inside the skill_view worker's snapshot: the
submitting thread's context never saw it, and the next tool's snapshot
(execute_code, terminal) always started from an empty allowlist — so a
skill's env vars were never passed through, on any turn, in any session
(#90004). The config-based allowlist (terminal.env_passthrough) is a
module-level global and worked, which is why manually adding the var
there was the only workaround.

Store the skill-registered names in a process-wide set, matching the
config path's visibility. Cross-session exposure is limited to the
names — values still resolve per profile through
resolve_passthrough_value's secret_scope, so a name registered by one
session cannot read another profile's secret. clear_env_passthrough
keeps its reset semantics; a later skill_view re-registers on demand.
The child_env fixture still reset the allowlist via the removed
_allowed_env_vars_var ContextVar token, so every test using the fixture
(AttributeError at setup) failed across all Python/OS lanes once #90004's
process-wide set landed. Save/restore the set contents instead.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/skills Skills system (list, view, manage) tool/terminal Terminal execution and process management tool/code-exec execute_code sandbox sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Oct 3, 2026
@OutThisLife
OutThisLife enabled auto-merge (rebase) October 5, 2026 21:40
@OutThisLife
OutThisLife merged commit e92da2f into main Oct 5, 2026
54 checks passed
@OutThisLife
OutThisLife deleted the bb/90004-land-90011 branch October 5, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/code-exec execute_code sandbox tool/skills Skills system (list, view, manage) 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.

Skill-declared env var (prerequisites.env_vars) not passed through to execute_code on first use

3 participants