mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Honor per-environment login shell policy (#36811)
## What changed - Store the effective `allow_login_shell` setting on each turn environment, including inherited environments whose child thread has a different policy. - Expose the `login` argument for shell tools when any selected environment permits login shells. - Validate each command against the policy of its selected environment instead of the turn-wide configuration. ## Testing - Cover tool schema generation for single and multiple environments. - Cover login-shell rejection by both the command handler and the unified exec integration. GitOrigin-RevId: 5a93149a5c86f4087f2b92d663ebc33feff8a57c
This commit is contained in:
1 parent
61dc1d97f6
commit
b258c028fe
18 files changed
+291
-28
No files matched your search
@@ -22,6 +22,7 @@ use futures::future::BoxFuture;
|
||||
use futures::future::Shared;
|
||||
use tokio_util::task::AbortOnDropHandle;
|
||||
|
||||
use crate::session::turn_context::EnvironmentConfig;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell_snapshot::ShellSnapshot;
|
||||
@@ -79,6 +80,8 @@ impl StartingTurnEnvironment {
|
||||
pub(crate) struct ThreadEnvironments {
|
||||
environment_manager: Arc<EnvironmentManager>,
|
||||
local_shell: Shell,
|
||||
// Temporary thread-wide config source until environments supply their own config.
|
||||
fallback_environment_config: EnvironmentConfig,
|
||||
shell_snapshot: ShellSnapshot,
|
||||
non_blocking_snapshots: bool,
|
||||
environments: ArcSwap<Vec<SelectedTurnEnvironment>>,
|
||||
@@ -89,6 +92,7 @@ impl ThreadEnvironments {
|
||||
pub(crate) fn new(
|
||||
environment_manager: Arc<EnvironmentManager>,
|
||||
local_shell: Shell,
|
||||
fallback_environment_config: EnvironmentConfig,
|
||||
shell_snapshot: ShellSnapshot,
|
||||
current: TurnEnvironmentSnapshot,
|
||||
non_blocking_snapshots: bool,
|
||||
@@ -98,9 +102,11 @@ impl ThreadEnvironments {
|
||||
.environments
|
||||
.into_iter()
|
||||
.filter_map(|environment| {
|
||||
let TurnEnvironmentState::Ready(environment) = environment else {
|
||||
let TurnEnvironmentState::Ready(mut environment) = environment else {
|
||||
return None;
|
||||
};
|
||||
// Child threads can have a different shell policy from their parent.
|
||||
environment.config = fallback_environment_config.clone();
|
||||
let selection = environment.selection();
|
||||
let selected_environment = Arc::clone(&environment.environment);
|
||||
let resolution: TurnEnvironmentResolution =
|
||||
@@ -116,6 +122,7 @@ impl ThreadEnvironments {
|
||||
Self {
|
||||
environment_manager,
|
||||
local_shell,
|
||||
fallback_environment_config,
|
||||
shell_snapshot,
|
||||
non_blocking_snapshots,
|
||||
environments: ArcSwap::from_pointee(environments),
|
||||
@@ -166,6 +173,7 @@ impl ThreadEnvironments {
|
||||
selected_environment.clone(),
|
||||
Arc::clone(&environment),
|
||||
self.local_shell.clone(),
|
||||
self.fallback_environment_config.clone(),
|
||||
self.shell_snapshot.clone(),
|
||||
)
|
||||
.remote_handle();
|
||||
@@ -265,6 +273,7 @@ impl ThreadEnvironments {
|
||||
selection: TurnEnvironmentSelection,
|
||||
environment: Arc<Environment>,
|
||||
local_shell: Shell,
|
||||
config: EnvironmentConfig,
|
||||
shell_snapshot: ShellSnapshot,
|
||||
) -> BoxFuture<'static, TurnEnvironmentResult> {
|
||||
async move {
|
||||
@@ -300,6 +309,7 @@ impl ThreadEnvironments {
|
||||
selection.cwd,
|
||||
selection.workspace_roots,
|
||||
shell,
|
||||
config,
|
||||
);
|
||||
let task = shell_snapshot
|
||||
.build(turn_environment.clone())
|
||||
@@ -506,6 +516,9 @@ mod tests {
|
||||
let turn_environments = Arc::new(ThreadEnvironments::new(
|
||||
environment_manager,
|
||||
crate::shell::default_user_shell(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot::default(),
|
||||
/*non_blocking_snapshots*/ false,
|
||||
@@ -649,7 +662,7 @@ url = "ws://127.0.0.1:8765"
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn local_environment_uses_configured_shell() {
|
||||
async fn local_environment_uses_configured_shell_and_login_policy() {
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let local_shell = Shell {
|
||||
shell_type: crate::shell::ShellType::Zsh,
|
||||
@@ -658,6 +671,9 @@ url = "ws://127.0.0.1:8765"
|
||||
let turn_environments = ThreadEnvironments::new(
|
||||
Arc::new(EnvironmentManager::default_for_tests()),
|
||||
local_shell.clone(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: false,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot::default(),
|
||||
/*non_blocking_snapshots*/ false,
|
||||
@@ -669,13 +685,10 @@ url = "ws://127.0.0.1:8765"
|
||||
}]);
|
||||
|
||||
let snapshot = turn_environments.snapshot().await;
|
||||
let environment = snapshot.primary().expect("local environment");
|
||||
|
||||
assert_eq!(
|
||||
snapshot
|
||||
.primary()
|
||||
.and_then(|environment| environment.shell.as_ref()),
|
||||
Some(&local_shell)
|
||||
);
|
||||
assert_eq!(environment.shell.as_ref(), Some(&local_shell));
|
||||
assert!(!environment.config.allow_login_shell);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -797,6 +810,9 @@ url = "ws://127.0.0.1:8765"
|
||||
let environments = Arc::new(ThreadEnvironments::new(
|
||||
manager,
|
||||
crate::shell::default_user_shell(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot::default(),
|
||||
/*non_blocking_snapshots*/ false,
|
||||
@@ -850,6 +866,9 @@ url = "ws://127.0.0.1:8765"
|
||||
let turn_environments = ThreadEnvironments::new(
|
||||
manager,
|
||||
crate::shell::default_user_shell(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot::default(),
|
||||
/*non_blocking_snapshots*/ true,
|
||||
@@ -924,6 +943,9 @@ url = "ws://127.0.0.1:8765"
|
||||
let environments = ThreadEnvironments::new(
|
||||
Arc::clone(&manager),
|
||||
crate::shell::default_user_shell(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot::default(),
|
||||
/*non_blocking_snapshots*/ true,
|
||||
@@ -978,6 +1000,9 @@ url = "ws://127.0.0.1:8765"
|
||||
let environments = Arc::new(ThreadEnvironments::new(
|
||||
Arc::clone(&manager),
|
||||
crate::shell::default_user_shell(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot::default(),
|
||||
/*non_blocking_snapshots*/ true,
|
||||
@@ -1050,7 +1075,7 @@ url = "ws://127.0.0.1:8765"
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn inherited_environment_reuses_parent_handle() {
|
||||
async fn inherited_environment_reuses_parent_handle_and_uses_child_login_policy() {
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let selection = TurnEnvironmentSelection {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
@@ -1067,6 +1092,9 @@ url = "ws://127.0.0.1:8765"
|
||||
selection.cwd.clone(),
|
||||
Vec::new(),
|
||||
/*shell*/ None,
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
);
|
||||
let manager = Arc::new(environment_manager_without_environments());
|
||||
manager
|
||||
@@ -1079,6 +1107,9 @@ url = "ws://127.0.0.1:8765"
|
||||
let environments = ThreadEnvironments::new(
|
||||
manager,
|
||||
crate::shell::default_user_shell(),
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: false,
|
||||
},
|
||||
ShellSnapshot::disabled(),
|
||||
TurnEnvironmentSnapshot {
|
||||
environments: vec![TurnEnvironmentState::Ready(inherited)],
|
||||
@@ -1089,13 +1120,9 @@ url = "ws://127.0.0.1:8765"
|
||||
environments.update_selections(std::slice::from_ref(&selection));
|
||||
let snapshot = environments.snapshot().await;
|
||||
|
||||
assert!(Arc::ptr_eq(
|
||||
&snapshot
|
||||
.primary()
|
||||
.expect("inherited environment")
|
||||
.environment,
|
||||
&inherited_environment,
|
||||
));
|
||||
let inherited = snapshot.primary().expect("inherited environment");
|
||||
assert!(Arc::ptr_eq(&inherited.environment, &inherited_environment));
|
||||
assert!(!inherited.config.allow_login_shell);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1124,6 +1151,9 @@ url = "ws://127.0.0.1:8765"
|
||||
cwd_uri.clone(),
|
||||
Vec::new(),
|
||||
/*shell*/ None,
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
))],
|
||||
};
|
||||
let multiple = TurnEnvironmentSnapshot {
|
||||
@@ -1135,6 +1165,9 @@ url = "ws://127.0.0.1:8765"
|
||||
cwd_uri,
|
||||
Vec::new(),
|
||||
/*shell*/ None,
|
||||
EnvironmentConfig {
|
||||
allow_login_shell: true,
|
||||
},
|
||||
)),
|
||||
],
|
||||
};
|
||||
|
||||
Reference in new issue
Block a user