fix(subagents) share execpolicy by default (#13702)
## Summary If a subagent requests approval, and the user persists that approval to the execpolicy, it should (by default) propagate. We'll need to rethink this a bit in light of coming Permissions changes, though I think this is closer to the end state that we'd want, which is that execpolicy changes to one permissions profile should be synced across threads. ## Testing - [x] Added integration test --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
parent
a3613035f3
commit
84f4e7b39d
10 changed files with 427 additions and 13 deletions
|
|
@ -107,6 +107,9 @@ impl AgentControl {
|
|||
let inherited_shell_snapshot = self
|
||||
.inherited_shell_snapshot_for_source(&state, session_source.as_ref())
|
||||
.await;
|
||||
let inherited_exec_policy = self
|
||||
.inherited_exec_policy_for_source(&state, session_source.as_ref(), &config)
|
||||
.await;
|
||||
let session_source = match session_source {
|
||||
Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn {
|
||||
parent_thread_id,
|
||||
|
|
@ -189,6 +192,7 @@ impl AgentControl {
|
|||
session_source,
|
||||
/*persist_extended_history*/ false,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
)
|
||||
.await?
|
||||
} else {
|
||||
|
|
@ -200,6 +204,7 @@ impl AgentControl {
|
|||
/*persist_extended_history*/ false,
|
||||
/*metrics_service_name*/ None,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
)
|
||||
.await?
|
||||
}
|
||||
|
|
@ -271,6 +276,9 @@ impl AgentControl {
|
|||
let inherited_shell_snapshot = self
|
||||
.inherited_shell_snapshot_for_source(&state, Some(&session_source))
|
||||
.await;
|
||||
let inherited_exec_policy = self
|
||||
.inherited_exec_policy_for_source(&state, Some(&session_source), &config)
|
||||
.await;
|
||||
let rollout_path =
|
||||
find_thread_path_by_id_str(config.codex_home.as_path(), &thread_id.to_string())
|
||||
.await?
|
||||
|
|
@ -283,6 +291,7 @@ impl AgentControl {
|
|||
self.clone(),
|
||||
session_source,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
)
|
||||
.await?;
|
||||
reservation.commit(resumed_thread.thread_id);
|
||||
|
|
@ -499,6 +508,30 @@ impl AgentControl {
|
|||
let parent_thread = state.get_thread(*parent_thread_id).await.ok()?;
|
||||
parent_thread.codex.session.user_shell().shell_snapshot()
|
||||
}
|
||||
|
||||
async fn inherited_exec_policy_for_source(
|
||||
&self,
|
||||
state: &Arc<ThreadManagerState>,
|
||||
session_source: Option<&SessionSource>,
|
||||
child_config: &crate::config::Config,
|
||||
) -> Option<Arc<crate::exec_policy::ExecPolicyManager>> {
|
||||
let Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn {
|
||||
parent_thread_id, ..
|
||||
})) = session_source
|
||||
else {
|
||||
return None;
|
||||
};
|
||||
|
||||
let parent_thread = state.get_thread(*parent_thread_id).await.ok()?;
|
||||
let parent_config = parent_thread.codex.session.get_config().await;
|
||||
if !crate::exec_policy::child_uses_parent_exec_policy(&parent_config, child_config) {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(Arc::clone(
|
||||
&parent_thread.codex.session.services.exec_policy,
|
||||
))
|
||||
}
|
||||
}
|
||||
#[cfg(test)]
|
||||
#[path = "control_tests.rs"]
|
||||
|
|
|
|||
|
|
@ -376,6 +376,7 @@ pub(crate) struct CodexSpawnArgs {
|
|||
pub(crate) persist_extended_history: bool,
|
||||
pub(crate) metrics_service_name: Option<String>,
|
||||
pub(crate) inherited_shell_snapshot: Option<Arc<ShellSnapshot>>,
|
||||
pub(crate) inherited_exec_policy: Option<Arc<ExecPolicyManager>>,
|
||||
pub(crate) user_shell_override: Option<shell::Shell>,
|
||||
pub(crate) parent_trace: Option<W3cTraceContext>,
|
||||
}
|
||||
|
|
@ -429,6 +430,7 @@ impl Codex {
|
|||
metrics_service_name,
|
||||
inherited_shell_snapshot,
|
||||
user_shell_override,
|
||||
inherited_exec_policy,
|
||||
parent_trace: _,
|
||||
} = args;
|
||||
let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY);
|
||||
|
|
@ -485,11 +487,15 @@ impl Codex {
|
|||
// Guardian review should rely on the built-in shell safety checks,
|
||||
// not on caller-provided exec-policy rules that could shape the
|
||||
// reviewer or silently auto-approve commands.
|
||||
ExecPolicyManager::default()
|
||||
Arc::new(ExecPolicyManager::default())
|
||||
} else if let Some(exec_policy) = &inherited_exec_policy {
|
||||
Arc::clone(exec_policy)
|
||||
} else {
|
||||
ExecPolicyManager::load(&config.config_layer_stack)
|
||||
.await
|
||||
.map_err(|err| CodexErr::Fatal(format!("failed to load rules: {err}")))?
|
||||
Arc::new(
|
||||
ExecPolicyManager::load(&config.config_layer_stack)
|
||||
.await
|
||||
.map_err(|err| CodexErr::Fatal(format!("failed to load rules: {err}")))?,
|
||||
)
|
||||
};
|
||||
|
||||
let config = Arc::new(config);
|
||||
|
|
@ -1386,7 +1392,7 @@ impl Session {
|
|||
config: Arc<Config>,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
models_manager: Arc<ModelsManager>,
|
||||
exec_policy: ExecPolicyManager,
|
||||
exec_policy: Arc<ExecPolicyManager>,
|
||||
tx_event: Sender<Event>,
|
||||
agent_status: watch::Sender<AgentStatus>,
|
||||
initial_history: InitialHistory,
|
||||
|
|
|
|||
|
|
@ -89,6 +89,7 @@ pub(crate) async fn run_codex_thread_interactive(
|
|||
metrics_service_name: None,
|
||||
inherited_shell_snapshot: None,
|
||||
user_shell_override: None,
|
||||
inherited_exec_policy: Some(Arc::clone(&parent_session.services.exec_policy)),
|
||||
parent_trace: None,
|
||||
})
|
||||
.await?;
|
||||
|
|
|
|||
|
|
@ -2364,7 +2364,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() {
|
|||
Arc::clone(&config),
|
||||
auth_manager,
|
||||
models_manager,
|
||||
ExecPolicyManager::default(),
|
||||
Arc::new(ExecPolicyManager::default()),
|
||||
tx_event,
|
||||
agent_status_tx,
|
||||
InitialHistory::New,
|
||||
|
|
@ -2400,7 +2400,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
|||
CollaborationModesConfig::default(),
|
||||
));
|
||||
let agent_control = AgentControl::default();
|
||||
let exec_policy = ExecPolicyManager::default();
|
||||
let exec_policy = Arc::new(ExecPolicyManager::default());
|
||||
let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit);
|
||||
let model = ModelsManager::get_model_offline_for_tests(config.model.as_deref());
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(model.as_str(), &config);
|
||||
|
|
@ -3194,7 +3194,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
|||
CollaborationModesConfig::default(),
|
||||
));
|
||||
let agent_control = AgentControl::default();
|
||||
let exec_policy = ExecPolicyManager::default();
|
||||
let exec_policy = Arc::new(ExecPolicyManager::default());
|
||||
let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit);
|
||||
let model = ModelsManager::get_model_offline_for_tests(config.model.as_deref());
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(model.as_str(), &config);
|
||||
|
|
|
|||
|
|
@ -452,6 +452,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() {
|
|||
persist_extended_history: false,
|
||||
metrics_service_name: None,
|
||||
inherited_shell_snapshot: None,
|
||||
inherited_exec_policy: Some(Arc::new(parent_exec_policy)),
|
||||
user_shell_override: None,
|
||||
parent_trace: None,
|
||||
})
|
||||
|
|
|
|||
|
|
@ -32,8 +32,10 @@ use tracing::instrument;
|
|||
|
||||
use crate::bash::parse_shell_lc_plain_commands;
|
||||
use crate::bash::parse_shell_lc_single_command_prefix;
|
||||
use crate::config::Config;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use shlex::try_join as shlex_try_join;
|
||||
|
||||
const PROMPT_CONFLICT_REASON: &str =
|
||||
|
|
@ -94,6 +96,24 @@ static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[
|
|||
&["osascript"],
|
||||
];
|
||||
|
||||
pub(crate) fn child_uses_parent_exec_policy(parent_config: &Config, child_config: &Config) -> bool {
|
||||
fn exec_policy_config_folders(config: &Config) -> Vec<AbsolutePathBuf> {
|
||||
config
|
||||
.config_layer_stack
|
||||
.get_layers(
|
||||
ConfigLayerStackOrdering::LowestPrecedenceFirst,
|
||||
/*include_disabled*/ false,
|
||||
)
|
||||
.into_iter()
|
||||
.filter_map(codex_config::ConfigLayerEntry::config_folder)
|
||||
.collect()
|
||||
}
|
||||
|
||||
exec_policy_config_folders(parent_config) == exec_policy_config_folders(child_config)
|
||||
&& parent_config.config_layer_stack.requirements().exec_policy
|
||||
== child_config.config_layer_stack.requirements().exec_policy
|
||||
}
|
||||
|
||||
fn is_policy_match(rule_match: &RuleMatch) -> bool {
|
||||
match rule_match {
|
||||
RuleMatch::PrefixRuleMatch { .. } => true,
|
||||
|
|
@ -170,6 +190,7 @@ pub enum ExecPolicyUpdateError {
|
|||
|
||||
pub(crate) struct ExecPolicyManager {
|
||||
policy: ArcSwap<Policy>,
|
||||
update_lock: tokio::sync::Mutex<()>,
|
||||
}
|
||||
|
||||
pub(crate) struct ExecApprovalRequest<'a> {
|
||||
|
|
@ -185,6 +206,7 @@ impl ExecPolicyManager {
|
|||
pub(crate) fn new(policy: Arc<Policy>) -> Self {
|
||||
Self {
|
||||
policy: ArcSwap::from(policy),
|
||||
update_lock: tokio::sync::Mutex::new(()),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -292,11 +314,11 @@ impl ExecPolicyManager {
|
|||
codex_home: &Path,
|
||||
amendment: &ExecPolicyAmendment,
|
||||
) -> Result<(), ExecPolicyUpdateError> {
|
||||
let _update_guard = self.update_lock.lock().await;
|
||||
let policy_path = default_policy_path(codex_home);
|
||||
let prefix = amendment.command.clone();
|
||||
spawn_blocking({
|
||||
let policy_path = policy_path.clone();
|
||||
let prefix = prefix.clone();
|
||||
let prefix = amendment.command.clone();
|
||||
move || blocking_append_allow_prefix_rule(&policy_path, &prefix)
|
||||
})
|
||||
.await
|
||||
|
|
@ -306,8 +328,25 @@ impl ExecPolicyManager {
|
|||
source,
|
||||
})?;
|
||||
|
||||
let mut updated_policy = self.current().as_ref().clone();
|
||||
updated_policy.add_prefix_rule(&prefix, Decision::Allow)?;
|
||||
let current_policy = self.current();
|
||||
let match_options = MatchOptions {
|
||||
resolve_host_executables: true,
|
||||
};
|
||||
let existing_evaluation = current_policy.check_multiple_with_options(
|
||||
[&amendment.command],
|
||||
&|_| Decision::Forbidden,
|
||||
&match_options,
|
||||
);
|
||||
let already_allowed = existing_evaluation.decision == Decision::Allow
|
||||
&& existing_evaluation.matched_rules.iter().any(|rule_match| {
|
||||
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
|
||||
});
|
||||
if already_allowed {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let mut updated_policy = current_policy.as_ref().clone();
|
||||
updated_policy.add_prefix_rule(&amendment.command, Decision::Allow)?;
|
||||
self.policy.store(Arc::new(updated_policy));
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -320,6 +359,7 @@ impl ExecPolicyManager {
|
|||
decision: Decision,
|
||||
justification: Option<String>,
|
||||
) -> Result<(), ExecPolicyUpdateError> {
|
||||
let _update_guard = self.update_lock.lock().await;
|
||||
let policy_path = default_policy_path(codex_home);
|
||||
let host = host.to_string();
|
||||
spawn_blocking({
|
||||
|
|
|
|||
|
|
@ -1,9 +1,16 @@
|
|||
use super::*;
|
||||
use crate::config::Config;
|
||||
use crate::config::ConfigBuilder;
|
||||
use crate::config_loader::ConfigLayerEntry;
|
||||
use crate::config_loader::ConfigLayerStack;
|
||||
use crate::config_loader::ConfigLayerStackOrdering;
|
||||
use crate::config_loader::ConfigRequirements;
|
||||
use crate::config_loader::ConfigRequirementsToml;
|
||||
use crate::config_loader::LoaderOverrides;
|
||||
use crate::config_loader::RequirementSource;
|
||||
use crate::config_loader::Sourced;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_config::RequirementsExecPolicy;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
|
|
@ -17,6 +24,7 @@ use std::fs;
|
|||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use tempfile::TempDir;
|
||||
use tempfile::tempdir;
|
||||
use toml::Value as TomlValue;
|
||||
|
||||
|
|
@ -73,6 +81,92 @@ fn unrestricted_file_system_sandbox_policy() -> FileSystemSandboxPolicy {
|
|||
FileSystemSandboxPolicy::unrestricted()
|
||||
}
|
||||
|
||||
async fn test_config() -> (TempDir, Config) {
|
||||
let home = TempDir::new().expect("create temp dir");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.loader_overrides(LoaderOverrides {
|
||||
#[cfg(target_os = "macos")]
|
||||
managed_preferences_base64: Some(String::new()),
|
||||
macos_managed_config_requirements_base64: Some(String::new()),
|
||||
..LoaderOverrides::default()
|
||||
})
|
||||
.build()
|
||||
.await
|
||||
.expect("load default test config");
|
||||
(home, config)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn child_uses_parent_exec_policy_when_layer_stack_matches() {
|
||||
let (_home, parent_config) = test_config().await;
|
||||
let child_config = parent_config.clone();
|
||||
|
||||
assert!(child_uses_parent_exec_policy(&parent_config, &child_config));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn child_uses_parent_exec_policy_when_non_exec_policy_layers_differ() {
|
||||
let (_home, parent_config) = test_config().await;
|
||||
let mut child_config = parent_config.clone();
|
||||
let mut layers: Vec<_> = child_config
|
||||
.config_layer_stack
|
||||
.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true)
|
||||
.into_iter()
|
||||
.cloned()
|
||||
.collect();
|
||||
layers.push(ConfigLayerEntry::new(
|
||||
ConfigLayerSource::SessionFlags,
|
||||
TomlValue::Table(Default::default()),
|
||||
));
|
||||
child_config.config_layer_stack = ConfigLayerStack::new(
|
||||
layers,
|
||||
child_config.config_layer_stack.requirements().clone(),
|
||||
child_config.config_layer_stack.requirements_toml().clone(),
|
||||
)
|
||||
.expect("config layer stack");
|
||||
|
||||
assert!(child_uses_parent_exec_policy(&parent_config, &child_config));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn child_does_not_use_parent_exec_policy_when_requirements_exec_policy_differs() {
|
||||
let (_home, parent_config) = test_config().await;
|
||||
let mut child_config = parent_config.clone();
|
||||
let mut requirements = ConfigRequirements {
|
||||
exec_policy: child_config
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.exec_policy
|
||||
.clone(),
|
||||
..ConfigRequirements::default()
|
||||
};
|
||||
let mut policy = Policy::empty();
|
||||
policy
|
||||
.add_prefix_rule(&["rm".to_string()], Decision::Forbidden)
|
||||
.expect("add prefix rule");
|
||||
requirements.exec_policy = Some(Sourced::new(
|
||||
RequirementsExecPolicy::new(policy),
|
||||
RequirementSource::Unknown,
|
||||
));
|
||||
child_config.config_layer_stack = ConfigLayerStack::new(
|
||||
child_config
|
||||
.config_layer_stack
|
||||
.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true)
|
||||
.into_iter()
|
||||
.cloned()
|
||||
.collect(),
|
||||
requirements,
|
||||
child_config.config_layer_stack.requirements_toml().clone(),
|
||||
)
|
||||
.expect("config layer stack");
|
||||
|
||||
assert!(!child_uses_parent_exec_policy(
|
||||
&parent_config,
|
||||
&child_config
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn returns_empty_policy_when_no_policy_files_exist() {
|
||||
let temp_dir = tempdir().expect("create temp dir");
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@ pub(crate) struct SessionServices {
|
|||
pub(crate) user_shell: Arc<crate::shell::Shell>,
|
||||
pub(crate) shell_snapshot_tx: watch::Sender<Option<Arc<crate::shell_snapshot::ShellSnapshot>>>,
|
||||
pub(crate) show_raw_agent_reasoning: bool,
|
||||
pub(crate) exec_policy: ExecPolicyManager,
|
||||
pub(crate) exec_policy: Arc<ExecPolicyManager>,
|
||||
pub(crate) auth_manager: Arc<AuthManager>,
|
||||
pub(crate) models_manager: Arc<ModelsManager>,
|
||||
pub(crate) session_telemetry: SessionTelemetry,
|
||||
|
|
|
|||
|
|
@ -610,10 +610,12 @@ impl ThreadManagerState {
|
|||
/*persist_extended_history*/ false,
|
||||
/*metrics_service_name*/ None,
|
||||
/*inherited_shell_snapshot*/ None,
|
||||
/*inherited_exec_policy*/ None,
|
||||
))
|
||||
.await
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(crate) async fn spawn_new_thread_with_source(
|
||||
&self,
|
||||
config: Config,
|
||||
|
|
@ -622,6 +624,7 @@ impl ThreadManagerState {
|
|||
persist_extended_history: bool,
|
||||
metrics_service_name: Option<String>,
|
||||
inherited_shell_snapshot: Option<Arc<ShellSnapshot>>,
|
||||
inherited_exec_policy: Option<Arc<crate::exec_policy::ExecPolicyManager>>,
|
||||
) -> CodexResult<NewThread> {
|
||||
Box::pin(self.spawn_thread_with_source(
|
||||
config,
|
||||
|
|
@ -633,6 +636,7 @@ impl ThreadManagerState {
|
|||
persist_extended_history,
|
||||
metrics_service_name,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
/*parent_trace*/ None,
|
||||
/*user_shell_override*/ None,
|
||||
))
|
||||
|
|
@ -646,6 +650,7 @@ impl ThreadManagerState {
|
|||
agent_control: AgentControl,
|
||||
session_source: SessionSource,
|
||||
inherited_shell_snapshot: Option<Arc<ShellSnapshot>>,
|
||||
inherited_exec_policy: Option<Arc<crate::exec_policy::ExecPolicyManager>>,
|
||||
) -> CodexResult<NewThread> {
|
||||
let initial_history = RolloutRecorder::get_rollout_history(&rollout_path).await?;
|
||||
Box::pin(self.spawn_thread_with_source(
|
||||
|
|
@ -658,12 +663,14 @@ impl ThreadManagerState {
|
|||
/*persist_extended_history*/ false,
|
||||
/*metrics_service_name*/ None,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
/*parent_trace*/ None,
|
||||
/*user_shell_override*/ None,
|
||||
))
|
||||
.await
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(crate) async fn fork_thread_with_source(
|
||||
&self,
|
||||
config: Config,
|
||||
|
|
@ -672,6 +679,7 @@ impl ThreadManagerState {
|
|||
session_source: SessionSource,
|
||||
persist_extended_history: bool,
|
||||
inherited_shell_snapshot: Option<Arc<ShellSnapshot>>,
|
||||
inherited_exec_policy: Option<Arc<crate::exec_policy::ExecPolicyManager>>,
|
||||
) -> CodexResult<NewThread> {
|
||||
Box::pin(self.spawn_thread_with_source(
|
||||
config,
|
||||
|
|
@ -683,6 +691,7 @@ impl ThreadManagerState {
|
|||
persist_extended_history,
|
||||
/*metrics_service_name*/ None,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
/*parent_trace*/ None,
|
||||
/*user_shell_override*/ None,
|
||||
))
|
||||
|
|
@ -713,6 +722,7 @@ impl ThreadManagerState {
|
|||
persist_extended_history,
|
||||
metrics_service_name,
|
||||
/*inherited_shell_snapshot*/ None,
|
||||
/*inherited_exec_policy*/ None,
|
||||
parent_trace,
|
||||
user_shell_override,
|
||||
))
|
||||
|
|
@ -731,6 +741,7 @@ impl ThreadManagerState {
|
|||
persist_extended_history: bool,
|
||||
metrics_service_name: Option<String>,
|
||||
inherited_shell_snapshot: Option<Arc<ShellSnapshot>>,
|
||||
inherited_exec_policy: Option<Arc<crate::exec_policy::ExecPolicyManager>>,
|
||||
parent_trace: Option<W3cTraceContext>,
|
||||
user_shell_override: Option<crate::shell::Shell>,
|
||||
) -> CodexResult<NewThread> {
|
||||
|
|
@ -754,6 +765,7 @@ impl ThreadManagerState {
|
|||
persist_extended_history,
|
||||
metrics_service_name,
|
||||
inherited_shell_snapshot,
|
||||
inherited_exec_policy,
|
||||
user_shell_override,
|
||||
parent_trace,
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
#![allow(clippy::unwrap_used, clippy::expect_used)]
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::CodexThread;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::config_loader::ConfigLayerStack;
|
||||
use codex_core::config_loader::ConfigLayerStackOrdering;
|
||||
|
|
@ -28,6 +29,7 @@ use core_test_support::responses::ev_completed;
|
|||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::mount_sse_once;
|
||||
use core_test_support::responses::mount_sse_once_match;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
|
|
@ -46,9 +48,11 @@ use std::env;
|
|||
use std::fs;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use tempfile::TempDir;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::Request;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
|
@ -681,6 +685,47 @@ async fn wait_for_completion(test: &TestCodex) {
|
|||
.await;
|
||||
}
|
||||
|
||||
fn body_contains(req: &Request, text: &str) -> bool {
|
||||
let is_zstd = req
|
||||
.headers
|
||||
.get("content-encoding")
|
||||
.and_then(|value| value.to_str().ok())
|
||||
.is_some_and(|value| {
|
||||
value
|
||||
.split(',')
|
||||
.any(|entry| entry.trim().eq_ignore_ascii_case("zstd"))
|
||||
});
|
||||
let bytes = if is_zstd {
|
||||
zstd::stream::decode_all(std::io::Cursor::new(&req.body)).ok()
|
||||
} else {
|
||||
Some(req.body.clone())
|
||||
};
|
||||
bytes
|
||||
.and_then(|body| String::from_utf8(body).ok())
|
||||
.is_some_and(|body| body.contains(text))
|
||||
}
|
||||
|
||||
async fn wait_for_spawned_thread(test: &TestCodex) -> Result<Arc<CodexThread>> {
|
||||
let deadline = tokio::time::Instant::now() + Duration::from_secs(2);
|
||||
loop {
|
||||
let ids = test.thread_manager.list_thread_ids().await;
|
||||
if let Some(thread_id) = ids
|
||||
.iter()
|
||||
.find(|id| **id != test.session_configured.session_id)
|
||||
{
|
||||
return test
|
||||
.thread_manager
|
||||
.get_thread(*thread_id)
|
||||
.await
|
||||
.map_err(anyhow::Error::from);
|
||||
}
|
||||
if tokio::time::Instant::now() >= deadline {
|
||||
anyhow::bail!("timed out waiting for spawned thread");
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
}
|
||||
}
|
||||
|
||||
fn scenarios() -> Vec<ScenarioSpec> {
|
||||
use AskForApproval::*;
|
||||
|
||||
|
|
@ -1996,6 +2041,188 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts
|
|||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn spawned_subagent_execpolicy_amendment_propagates_to_parent_session() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::UnlessTrusted;
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Collab)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
const PARENT_PROMPT: &str = "spawn a child that repeats a command";
|
||||
const CHILD_PROMPT: &str = "run the same command twice";
|
||||
const SPAWN_CALL_ID: &str = "spawn-child-1";
|
||||
const CHILD_CALL_ID_1: &str = "child-touch-1";
|
||||
const PARENT_CALL_ID_2: &str = "parent-touch-2";
|
||||
|
||||
let child_file = test.cwd.path().join("subagent-allow-prefix.txt");
|
||||
let _ = fs::remove_file(&child_file);
|
||||
|
||||
let spawn_args = serde_json::to_string(&json!({
|
||||
"message": CHILD_PROMPT,
|
||||
}))?;
|
||||
mount_sse_once_match(
|
||||
&server,
|
||||
|req: &Request| body_contains(req, PARENT_PROMPT),
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-1"),
|
||||
ev_function_call(SPAWN_CALL_ID, "spawn_agent", &spawn_args),
|
||||
ev_completed("resp-parent-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let child_cmd_args = serde_json::to_string(&json!({
|
||||
"command": "touch subagent-allow-prefix.txt",
|
||||
"timeout_ms": 1_000,
|
||||
"prefix_rule": ["touch", "subagent-allow-prefix.txt"],
|
||||
}))?;
|
||||
mount_sse_once_match(
|
||||
&server,
|
||||
|req: &Request| body_contains(req, CHILD_PROMPT) && !body_contains(req, SPAWN_CALL_ID),
|
||||
sse(vec![
|
||||
ev_response_created("resp-child-1"),
|
||||
ev_function_call(CHILD_CALL_ID_1, "shell_command", &child_cmd_args),
|
||||
ev_completed("resp-child-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
mount_sse_once_match(
|
||||
&server,
|
||||
|req: &Request| body_contains(req, CHILD_CALL_ID_1),
|
||||
sse(vec![
|
||||
ev_response_created("resp-child-2"),
|
||||
ev_assistant_message("msg-child-2", "child done"),
|
||||
ev_completed("resp-child-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
mount_sse_once_match(
|
||||
&server,
|
||||
|req: &Request| body_contains(req, SPAWN_CALL_ID),
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-2"),
|
||||
ev_assistant_message("msg-parent-2", "parent done"),
|
||||
ev_completed("resp-parent-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let _ = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-3"),
|
||||
ev_function_call(PARENT_CALL_ID_2, "shell_command", &child_cmd_args),
|
||||
ev_completed("resp-parent-3"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let _ = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-4"),
|
||||
ev_assistant_message("msg-parent-4", "parent rerun done"),
|
||||
ev_completed("resp-parent-4"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
PARENT_PROMPT,
|
||||
approval_policy,
|
||||
sandbox_policy.clone(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let child = wait_for_spawned_thread(&test).await?;
|
||||
let approval_event = wait_for_event_with_timeout(
|
||||
&child,
|
||||
|event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
},
|
||||
Duration::from_secs(2),
|
||||
)
|
||||
.await;
|
||||
|
||||
let EventMsg::ExecApprovalRequest(approval) = approval_event else {
|
||||
panic!("expected child approval before completion");
|
||||
};
|
||||
let expected_execpolicy_amendment = ExecPolicyAmendment::new(vec![
|
||||
"touch".to_string(),
|
||||
"subagent-allow-prefix.txt".to_string(),
|
||||
]);
|
||||
assert_eq!(
|
||||
approval.proposed_execpolicy_amendment,
|
||||
Some(expected_execpolicy_amendment.clone())
|
||||
);
|
||||
|
||||
child
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: expected_execpolicy_amendment,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let child_event = wait_for_event_with_timeout(
|
||||
&child,
|
||||
|event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
},
|
||||
Duration::from_secs(2),
|
||||
)
|
||||
.await;
|
||||
match child_event {
|
||||
EventMsg::TurnComplete(_) => {}
|
||||
EventMsg::ExecApprovalRequest(ev) => {
|
||||
panic!("unexpected second child approval request: {:?}", ev.command)
|
||||
}
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
assert!(
|
||||
child_file.exists(),
|
||||
"expected subagent command to create file"
|
||||
);
|
||||
fs::remove_file(&child_file)?;
|
||||
assert!(
|
||||
!child_file.exists(),
|
||||
"expected child file to be removed before parent rerun"
|
||||
);
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"parent reruns child command",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
)
|
||||
.await?;
|
||||
wait_for_completion_without_approval(&test).await;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[cfg(unix)]
|
||||
async fn matched_prefix_rule_runs_unsandboxed_under_zsh_fork() -> Result<()> {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue