Skip to content

fix(tools): forward set_skill_env and set_effective_trust through CompositeExecutor#3870

Merged
bug-ops merged 1 commit into
mainfrom
3869-composite-skill-env
May 13, 2026
Merged

fix(tools): forward set_skill_env and set_effective_trust through CompositeExecutor#3870
bug-ops merged 1 commit into
mainfrom
3869-composite-skill-env

Conversation

@bug-ops
Copy link
Copy Markdown
Owner

@bug-ops bug-ops commented May 13, 2026

Summary

  • CompositeExecutor now forwards set_skill_env and set_effective_trust to both inner executors.
  • Without this override, the base ToolExecutor impls (no-ops) were inherited, silently swallowing both calls at the very first CompositeExecutor boundary — which in production is always the outermost layer (src/agent_setup.rs:534-541).
  • Concrete impact:
    • Skill secret env injection broken: x-requires-secrets declarations parsed correctly and available_custom_secrets populated, but ShellExecutor::skill_env stayed None, so GITHUB_TOKEN (and any other declared secret) was never present in the bash subprocess env. Verified live in TUI: skill github active at confidence 0.98, env | grep GITHUB → NO_TOKEN.
    • Trust-gate enforcement bypassed: TrustGateExecutor::effective_trust stayed at the initial Trusted regardless of the active skill's trust level. QUARANTINE_DENIED tools (bash, write, web_scrape, etc.) were reachable while a quarantined skill was active.
  • Added two regression tests in composite::tests::state_forwarding using a nested CompositeExecutor tree and spy executors at every leaf, asserting both setters land on all leaves.

Closes #3869.

Test plan

  • cargo +nightly fmt --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo nextest run -p zeph-tools --lib --bins → 1174 passed (incl. 2 new regressions)
  • cargo nextest run --workspace --lib --bins → 9322 passed
  • Manual: rerun the original live-test scenario from CI-783 (TUI + bash $ env | grep GITHUB) and confirm GITHUB_TOKEN now present when github skill is active.

Notes

  • The fix is a forwarding override on CompositeExecutor, not a behavior change in any leaf executor. The defaults in the ToolExecutor trait remain no-ops — only intermediate composition no longer drops the call.
  • No public API surface changes. Trait method signatures are unchanged.

…positeExecutor

The base ToolExecutor impls of set_skill_env and set_effective_trust are no-ops.
CompositeExecutor relied on those defaults, so the production layered executor
(agent_setup builds nested CompositeExecutor trees) silently swallowed both calls.
Concrete impact:

- Skill secret env injection (x-requires-secrets) never reached ShellExecutor:
  GITHUB_TOKEN, AWS_*, etc. were absent from bash subprocesses even though the
  skill was active and the secret was loaded into available_custom_secrets.
- TrustGateExecutor never observed a non-Trusted level while a quarantined skill
  was active, leaving QUARANTINE_DENIED tools (bash, write, web_scrape, ...)
  reachable from quarantined contexts.

Override both setters on CompositeExecutor to forward the call to first AND
second; nested compositions propagate automatically. Add two regression tests
using a nested composition with spy executors at every leaf.

Closes #3869.
@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation rust Rust code changes size/M Medium PR (51-200 lines) and removed bug Something isn't working labels May 13, 2026
@bug-ops bug-ops enabled auto-merge (squash) May 13, 2026 15:31
@bug-ops bug-ops merged commit 1a78bf2 into main May 13, 2026
32 checks passed
@bug-ops bug-ops deleted the 3869-composite-skill-env branch May 13, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation rust Rust code changes size/M Medium PR (51-200 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

skill required-secrets env injection silently no-ops through CompositeExecutor

1 participant