From 620e307faaa38cc0e27cb8f10e80ec8e711b5b9b Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Wed, 13 May 2026 17:29:29 +0200 Subject: [PATCH] fix(tools): forward set_skill_env and set_effective_trust through CompositeExecutor 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. --- CHANGELOG.md | 7 +++ crates/zeph-tools/src/composite.rs | 96 ++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5ec1f73f..8ec76bb35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). with all tasks stuck in `Pending` (closes #3841). - `zeph-orchestration`: add `Llm(#[from] zeph_llm::LlmError)` typed variant to `OrchestrationError` so callers can pattern-match on root LLM error kinds without string comparison (closes #3842). +- `zeph-tools`: `CompositeExecutor` now forwards `set_skill_env` and `set_effective_trust` to both + inner executors. The base `ToolExecutor` impls of these setters are no-ops, so the production + layered executor (`agent_setup` builds a tree of `CompositeExecutor`s) silently swallowed both + calls — breaking `x-requires-secrets` env injection (e.g. `GITHUB_TOKEN` never reached the + `bash` subprocess) and bypassing `TrustGateExecutor`'s quarantine enforcement for active + skills. Added two regression tests using a nested composition and spy executors + (closes #3869). ### Changed diff --git a/crates/zeph-tools/src/composite.rs b/crates/zeph-tools/src/composite.rs index ce5b9f679..e80e8d5a7 100644 --- a/crates/zeph-tools/src/composite.rs +++ b/crates/zeph-tools/src/composite.rs @@ -85,6 +85,28 @@ impl ToolExecutor for CompositeExecutor fn is_tool_speculatable(&self, tool_id: &str) -> bool { self.first.is_tool_speculatable(tool_id) || self.second.is_tool_speculatable(tool_id) } + + /// Forward the active skill's env injection to BOTH inner executors. + /// + /// The base [`ToolExecutor::set_skill_env`] is a no-op, so without this override the + /// composition tree built in `agent_setup` silently swallows env injection — the + /// underlying `ShellExecutor` never sees `GITHUB_TOKEN` etc. Each layer ignores the call + /// if it does not own a `skill_env` slot; layers that do (e.g. `ShellExecutor`) update + /// their state. See #3869. + fn set_skill_env(&self, env: Option>) { + self.first.set_skill_env(env.clone()); + self.second.set_skill_env(env); + } + + /// Forward the active skill's trust level to BOTH inner executors. + /// + /// Mirrors [`Self::set_skill_env`]: without this override, `TrustGateExecutor` never + /// observes a non-`Trusted` level when composed under `CompositeExecutor`, leaving + /// quarantine enforcement effectively bypassed. See #3869. + fn set_effective_trust(&self, level: crate::SkillTrustLevel) { + self.first.set_effective_trust(level); + self.second.set_effective_trust(level); + } } #[cfg(test)] @@ -306,4 +328,78 @@ mod tests { let result = composite.execute_tool_call(&call).await.unwrap(); assert!(result.is_none()); } + + /// Regression test for #3869: state-mutating setters MUST reach both inner executors, + /// even across nested compositions. Prior to the fix, `set_skill_env` and + /// `set_effective_trust` fell through to the default no-op `ToolExecutor` impls and + /// were silently dropped at the `CompositeExecutor` boundary — breaking skill secret + /// env injection (`x-requires-secrets`) and quarantine trust enforcement. + mod state_forwarding { + use super::*; + use crate::SkillTrustLevel; + use std::sync::Mutex; + + #[derive(Debug, Default)] + struct SpyExecutor { + last_env: Mutex>>, + last_trust: Mutex>, + } + impl ToolExecutor for SpyExecutor { + async fn execute(&self, _: &str) -> Result, ToolError> { + Ok(None) + } + fn set_skill_env(&self, env: Option>) { + *self.last_env.lock().unwrap() = env; + } + fn set_effective_trust(&self, level: SkillTrustLevel) { + *self.last_trust.lock().unwrap() = Some(level); + } + } + + #[test] + fn set_skill_env_reaches_both_inner_executors_in_nested_composition() { + // Mirrors the production wiring shape: a tree of CompositeExecutor with + // multiple leaves. All leaves must observe the call. + let leaf_a = SpyExecutor::default(); + let leaf_b = SpyExecutor::default(); + let leaf_c = SpyExecutor::default(); + let nested = CompositeExecutor::new(leaf_a, leaf_b); + let outer = CompositeExecutor::new(nested, leaf_c); + + let mut env = std::collections::HashMap::new(); + env.insert("GITHUB_TOKEN".to_owned(), "tok".to_owned()); + outer.set_skill_env(Some(env.clone())); + + // first.first (leaf_a) + assert_eq!( + outer.first.first.last_env.lock().unwrap().as_ref(), + Some(&env) + ); + // first.second (leaf_b) + assert_eq!( + outer.first.second.last_env.lock().unwrap().as_ref(), + Some(&env) + ); + // second (leaf_c) + assert_eq!(outer.second.last_env.lock().unwrap().as_ref(), Some(&env)); + } + + #[test] + fn set_effective_trust_reaches_both_inner_executors_in_nested_composition() { + let leaf_a = SpyExecutor::default(); + let leaf_b = SpyExecutor::default(); + let outer = CompositeExecutor::new(leaf_a, leaf_b); + + outer.set_effective_trust(SkillTrustLevel::Quarantined); + + assert_eq!( + *outer.first.last_trust.lock().unwrap(), + Some(SkillTrustLevel::Quarantined) + ); + assert_eq!( + *outer.second.last_trust.lock().unwrap(), + Some(SkillTrustLevel::Quarantined) + ); + } + } }