Skip to content

fix(skills): make env_passthrough registrations visible across tool context snapshots - #90011

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-90004
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-90004

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

Fixes #90004: a skill's declared env vars (required_environment_variables and the legacy prerequisites.env_vars) never reached execute_code/terminal, on any turn or session — only manually adding the var to terminal.env_passthrough in config.yaml worked.

Mechanism. skill_view registers the skill's vars into the sandbox passthrough allowlist, which was backed by a ContextVar. 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 (a ContextVar.set in a copied context cannot reach the parent), and
  • the next tool's submit-time snapshot (execute_code, terminal) always started from an empty allowlist.

So the failure was deterministic, which matches the report exactly (consistent across fresh sessions; restart doesn't help). The config-based allowlist (terminal.env_passthrough) is a module-level global — that visibility difference is precisely why the manual workaround worked. I reproduced the invisibility in isolation on unmodified main (register inside a copied context, then a sibling snapshot sees False).

Fix. 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.

This un-breaks at least the 4 bundled skills the report lists as using the legacy key (notion, airtable, gif-search, teams-meeting-pipeline) — and equally affects skills using the modern required_environment_variables key, since both feed the same registration call.

Related Issue

Fixes #90004

Type of Change

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

Changes Made

  • tools/env_passthrough.py: the skill-registration allowlist moves from a ContextVar-backed set to a process-wide module-level set (_get_allowed returns it; register_env_passthrough mutates it in place; clear_env_passthrough clears it). The unused ContextVar import is dropped; comments document why process-wide visibility is required by the tool-dispatch snapshot model and what the exposure boundary is.

How to Test

  1. pytest tests/tools/test_env_passthrough.py -q — 25 passed (2 new tests).
  2. Fail-on-main verified for the core scenario: test_registration_in_copied_context_is_visible_to_sibling_snapshot runs the register-then-check sequence inside a pristine context (so the fixture's current-context initialization cannot mask the bug) — it fails on unmodified main and passes with the fix.
  3. tests/tools/test_skill_env_passthrough.py + tests/tools/test_local_env_blocklist.py: 104 passed, 2 skipped — the provider-credential blocklist (GHSA-rhgp-j443-p4rf) and profile-scope semantics are unchanged.
  4. tests/tools/test_code_execution*.py: 98 passed, 3 skipped; the single interrupt-handling failure reproduces identically on unmodified main (pre-existing, unrelated).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) tool/terminal Terminal execution and process management tool/code-exec execute_code sandbox area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Aug 19, 2026
@liuhao1024

Copy link
Copy Markdown
Author

The failing checks here are a pre-existing main regression, not caused by this diff: the failing test is the FAL catalog all-off upscale invariant (test_upscale_defaults_are_all_off), which was broken on upstream/main itself for a window on Aug 19 (a catalog entry shipped upscale: True against the opt-in-only policy the test pins). The same failure hit sibling PRs from that window and was root-caused on #89968/#89987.

Main has since resolved it on its own (the catalog entry was fixed and the invariant test was dropped in 00888e8), so this failure should disappear on the next rebase. This PR's changes don't touch the image-generation catalog or its tests. Not re-running CI since the failing slice is unrelated to the change under review.

…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
(NousResearch#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.

This branch has not been deployed

No deployments
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 P2 Medium — degraded but workaround exists 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

2 participants