From 83637ad7eae6cfa8ae634d495fcc362e3e91ccef Mon Sep 17 00:00:00 2001 From: Yuedong Wu Date: Thu, 25 Jun 2026 14:25:43 +0800 Subject: [PATCH] refactor(cli): replace sandbox_create positional args with SandboxCreateConfig struct Signed-off-by: Yuedong Wu --- crates/openshell-cli/src/main.rs | 38 +- crates/openshell-cli/src/run.rs | 104 +++-- .../sandbox_create_lifecycle_integration.rs | 368 ++++++------------ 3 files changed, 210 insertions(+), 300 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index fbed00e1a..1e72f0ba5 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -2678,25 +2678,27 @@ async fn main() -> Result<()> { apply_auth(&mut tls, &ctx.name); Box::pin(run::sandbox_create( endpoint, - name.as_deref(), - from.as_deref(), &ctx.name, - &upload_specs, - keep, - gpu_requirements, - cpu.as_deref(), - memory.as_deref(), - driver_config_json.as_deref(), - editor, - &providers, - policy.as_deref(), - forward, - &command, - tty_override, - auto_providers_override, - &labels_map, - &env_map, - &approval_mode, + run::SandboxCreateConfig { + name: name.as_deref(), + from: from.as_deref(), + uploads: &upload_specs, + keep, + gpu_requirements, + cpu: cpu.as_deref(), + memory: memory.as_deref(), + driver_config_json: driver_config_json.as_deref(), + editor, + providers: &providers, + policy: policy.as_deref(), + forward, + command: &command, + tty_override, + auto_providers_override, + labels: labels_map, + environment: env_map, + approval_mode: &approval_mode, + }, &tls, )) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 1c3fd8a82..2dbc3da73 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -77,7 +77,7 @@ pub use crate::ssh::{ sandbox_ssh_proxy_by_name, sandbox_sync_down, sandbox_sync_up, sandbox_sync_up_files, }; pub use openshell_core::forward::{ - find_forward_by_port, list_forwards, stop_forward, stop_forwards_for_sandbox, + ForwardSpec, find_forward_by_port, list_forwards, stop_forward, stop_forwards_for_sandbox, }; #[derive(Debug, PartialEq, Eq)] @@ -1574,10 +1574,7 @@ pub fn doctor_check() -> Result<()> { Err(miette::miette!("docker info failed: {}", stderr.trim())) } -fn sandbox_should_persist( - keep: bool, - forward: Option<&openshell_core::forward::ForwardSpec>, -) -> bool { +fn sandbox_should_persist(keep: bool, forward: Option<&ForwardSpec>) -> bool { keep || forward.is_some() } @@ -1745,31 +1742,86 @@ async fn finalize_sandbox_create_session( session_result } +/// Configuration for creating a sandbox via the CLI. +/// +/// Infrastructure parameters (`server`, `gateway_name`, `tls`) remain positional +/// on the function signature, following the `provider_refresh_config(server, input, tls)` +/// precedent. This struct captures sandbox-specific options. +#[derive(Debug)] +pub struct SandboxCreateConfig<'a> { + pub name: Option<&'a str>, + pub from: Option<&'a str>, + pub uploads: &'a [(String, Option, bool)], + pub keep: bool, + pub gpu_requirements: Option, + pub cpu: Option<&'a str>, + pub memory: Option<&'a str>, + pub driver_config_json: Option<&'a str>, + pub editor: Option, + pub providers: &'a [String], + pub policy: Option<&'a str>, + pub forward: Option, + pub command: &'a [String], + pub tty_override: Option, + pub auto_providers_override: Option, + pub labels: HashMap, + pub environment: HashMap, + pub approval_mode: &'a str, +} + +impl Default for SandboxCreateConfig<'_> { + fn default() -> Self { + Self { + name: None, + from: None, + uploads: &[], + keep: false, + gpu_requirements: None, + cpu: None, + memory: None, + driver_config_json: None, + editor: None, + providers: &[], + policy: None, + forward: None, + command: &[], + tty_override: None, + auto_providers_override: None, + labels: HashMap::new(), + environment: HashMap::new(), + approval_mode: "manual", + } + } +} + /// Create a sandbox with default settings. -#[allow(clippy::too_many_arguments, clippy::implicit_hasher)] // user-facing CLI command; default hasher is fine pub async fn sandbox_create( server: &str, - name: Option<&str>, - from: Option<&str>, gateway_name: &str, - uploads: &[(String, Option, bool)], - keep: bool, - gpu_requirements: Option, - cpu: Option<&str>, - memory: Option<&str>, - driver_config_json: Option<&str>, - editor: Option, - providers: &[String], - policy: Option<&str>, - forward: Option, - command: &[String], - tty_override: Option, - auto_providers_override: Option, - labels: &HashMap, - environment: &HashMap, - approval_mode: &str, + config: SandboxCreateConfig<'_>, tls: &TlsOptions, ) -> Result<()> { + let SandboxCreateConfig { + name, + from, + uploads, + keep, + gpu_requirements, + cpu, + memory, + driver_config_json, + editor, + providers, + policy, + forward, + command, + tty_override, + auto_providers_override, + labels, + environment, + approval_mode, + } = config; + if editor.is_some() && !command.is_empty() { return Err(miette::miette!( "--editor cannot be used with a trailing command; use `openshell sandbox connect --editor ...` after the sandbox is ready" @@ -1846,14 +1898,14 @@ pub async fn sandbox_create( let request = CreateSandboxRequest { spec: Some(SandboxSpec { resource_requirements, - environment: environment.clone(), + environment, policy, providers: configured_providers, template, ..SandboxSpec::default() }), name: name.unwrap_or_default().to_string(), - labels: labels.clone(), + labels, }; let response = match client.create_sandbox(request).await { diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 207386b84..5fd5844b6 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -1099,6 +1099,19 @@ fn gpu_requirements(count: Option) -> GpuResourceRequirements { GpuResourceRequirements { count } } +/// Shared defaults for integration tests. Note: `keep` is `true` here (most +/// tests expect persistent sandboxes) while `SandboxCreateConfig::default()` +/// sets `keep: false` (the safe production default). Tests that exercise +/// ephemeral behavior must explicitly override with `keep: false`. +fn test_config() -> run::SandboxCreateConfig<'static> { + run::SandboxCreateConfig { + keep: true, + tty_override: Some(false), + auto_providers_override: Some(false), + ..Default::default() + } +} + #[tokio::test] async fn sandbox_create_keeps_command_sessions_by_default() { let server = run_server().await; @@ -1108,27 +1121,15 @@ async fn sandbox_create_keeps_command_sessions_by_default() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("default-command"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("default-command"), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1151,27 +1152,17 @@ async fn sandbox_create_sends_cpu_and_memory_limits_only() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("resources"), - None, "openshell", - &[], - true, - None, - Some("500m"), - Some("2Gi"), - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("resources"), + cpu: Some("500m"), + memory: Some("2Gi"), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1228,27 +1219,18 @@ async fn sandbox_create_sends_driver_config_json() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("driver-config"), - None, "openshell", - &[], - true, - None, - None, - None, - Some(r#"{"kubernetes":{"pod":{"priority_class_name":"batch-low"}}}"#), - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("driver-config"), + driver_config_json: Some( + r#"{"kubernetes":{"pod":{"priority_class_name":"batch-low"}}}"#, + ), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1301,27 +1283,16 @@ async fn sandbox_create_sends_gpu_default_request() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("gpu-default"), - None, "openshell", - &[], - true, - Some(gpu_requirements(None)), - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("gpu-default"), + gpu_requirements: Some(gpu_requirements(None)), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1347,27 +1318,16 @@ async fn sandbox_create_sends_gpu_count_request() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("gpu-two"), - None, "openshell", - &[], - true, - Some(gpu_requirements(Some(2))), - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("gpu-two"), + gpu_requirements: Some(gpu_requirements(Some(2))), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1394,27 +1354,16 @@ async fn sandbox_create_does_not_infer_command_providers_when_v2_enabled() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["claude".into(), "--version".into()]; run::sandbox_create( &server.endpoint, - Some("v2-no-inferred-provider"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["claude".to_string(), "--version".to_string()], - Some(true), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("v2-no-inferred-provider"), + command: &cmd, + tty_override: Some(true), + ..test_config() + }, &tls, ) .await @@ -1451,28 +1400,16 @@ async fn sandbox_create_returns_vm_error_without_waiting_for_timeout() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; let started_at = Instant::now(); let err = run::sandbox_create( &server.endpoint, - Some("vm-error"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("vm-error"), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1506,27 +1443,15 @@ async fn sandbox_create_keeps_waiting_while_vm_progress_arrives() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("vm-slow-progress"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("vm-slow-progress"), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1551,28 +1476,16 @@ async fn sandbox_create_times_out_when_only_logs_arrive() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; let started_at = Instant::now(); let err = run::sandbox_create( &server.endpoint, - Some("vm-log-churn"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("vm-log-churn"), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1594,27 +1507,16 @@ async fn sandbox_create_deletes_command_sessions_with_no_keep() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("ephemeral-command"), - None, "openshell", - &[], - false, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("ephemeral-command"), + keep: false, + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1642,25 +1544,13 @@ async fn sandbox_create_deletes_shell_sessions_with_no_keep() { run::sandbox_create( &server.endpoint, - Some("ephemeral-shell"), - None, "openshell", - &[], - false, - None, - None, - None, - None, - None, - &[], - None, - None, - &[], - Some(true), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("ephemeral-shell"), + keep: false, + tty_override: Some(true), + ..test_config() + }, &tls, ) .await @@ -1686,27 +1576,15 @@ async fn sandbox_create_keeps_sandbox_with_hidden_keep_flag() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("persistent-keep"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("persistent-keep"), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1732,27 +1610,17 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { let forward_port = listener.local_addr().unwrap().port(); drop(listener); + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("persistent-forward"), - None, "openshell", - &[], - false, - None, - None, - None, - None, - None, - &[], - None, - Some(openshell_core::forward::ForwardSpec::new(forward_port)), - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &HashMap::new(), - "manual", + run::SandboxCreateConfig { + name: Some("persistent-forward"), + keep: false, + forward: Some(openshell_core::forward::ForwardSpec::new(forward_port)), + command: &cmd, + ..test_config() + }, &tls, ) .await @@ -1880,31 +1748,19 @@ async fn sandbox_create_sends_environment_variables() { let tls = test_tls(&server); install_fake_ssh(&fake_ssh_dir); - let mut env_map = HashMap::new(); - env_map.insert("FOO".to_string(), "bar".to_string()); - env_map.insert("BAZ".to_string(), "qux=with=equals".to_string()); - + let cmd = vec!["echo".into(), "OK".into()]; run::sandbox_create( &server.endpoint, - Some("env-test"), - None, "openshell", - &[], - true, - None, - None, - None, - None, - None, - &[], - None, - None, - &["echo".to_string(), "OK".to_string()], - Some(false), - Some(false), - &HashMap::new(), - &env_map, - "manual", + run::SandboxCreateConfig { + name: Some("env-test"), + command: &cmd, + environment: HashMap::from([ + ("FOO".into(), "bar".into()), + ("BAZ".into(), "qux=with=equals".into()), + ]), + ..test_config() + }, &tls, ) .await