chore: refactor sandbox_create to use a builder pattern

Description

run::sandbox_create in crates/openshell-cli/src/run.rs has grown into a large positional-argument API. Call sites now pass many None, false, Some(false), empty slice, and empty map values, which makes tests and CLI dispatch hard to read and easy to break when parameters are added or reordered.

Example current call shape:

run::sandbox_create(
    &server.endpoint,
    Some("vm-error"),
    None,
    "openshell",
    None,
    true,
    false,
    None,
    None,
    None,
    None,
    &[],
    None,
    None,
    &["echo".to_string(), "OK".to_string()],
    Some(false),
    Some(false),
    &HashMap::new(),
    &tls,
)

Context

Observed locations:

crates/openshell-cli/src/run.rs: sandbox_create currently takes 19 parameters and has #[allow(clippy::too_many_arguments)].
crates/openshell-cli/src/main.rs: CLI dispatch builds the positional call from parsed command state.
crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs: integration tests repeat long calls where only a few fields vary.

Proposed Work

Introduce a typed SandboxCreateOptions/SandboxCreateBuilder API for sandbox creation, then update the CLI and tests to construct options by field name instead of relying on positional arguments.

Possible shape:

run::SandboxCreateBuilder::new(&server.endpoint, &tls)
    .gateway_name("openshell")
    .name("vm-error")
    .keep(true)
    .command(["echo", "OK"])
    .tty_override(false)
    .auto_providers_override(false)
    .create()
    .await?;

The exact type names and ownership model should follow the existing CLI code style. Preserve existing sandbox_create behavior while moving the argument list into a single structured config.

Definition of Done

Replace the large positional sandbox_create signature with a builder/options-based API.
Remove the local clippy::too_many_arguments allowance for this method.
Update crates/openshell-cli/src/main.rs to construct the request through named fields/builder methods.
Update integration tests to use the new builder or concise test helpers.
Preserve existing sandbox create behavior, including keep/no-keep, forwarding, provider inference, resource limits, labels, TLS, and command handling.
Run the relevant CLI tests, at minimum cargo test -p openshell-cli sandbox_create or the equivalent mise task.

This issue has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. Comment or remove the state:stale label to keep it open.

@drew

Hi @drew, I'd like to take this task if it's still needed and open to external contribution

That'd be great, thanks.

@drew

While digging through the CLI crate codebase, I noticed after this issue was filed, PR #1349 landed and introduced ProviderRefreshConfigInput (consumed by provider_refresh_config(server, input, tls)):

OpenShell/crates/openshell-cli/src/run.rs

Lines 5115 to 5122 in f1245a3

    
           pub struct ProviderRefreshConfigInput<'a> { 
        
               pub name: &'a str, 
        
               pub credential_key: &'a str, 
        
               pub strategy: &'a str, 
        
               pub material: &'a [String], 
        
               pub secret_material_keys: &'a [String], 
        
               pub credential_expires_at_ms: Option<i64>, 
        
           }

which uses a struct-based config (plain struct with pub fields, no builder methods), not the builder pattern this issue originally sketched.

I consulted the spike agent, it suggests following ProviderRefreshConfigInput's config-bundle precedent:

run::sandbox_create(
    endpoint,
    &ctx.name,
    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?;

Hi @drew. Which approach should we go with? Do you have a preference? Any inputs would be appreciated. :)

Navigation Menu

Repository navigation

chore: refactor sandbox_create to use a builder pattern #1408

Description

Context

Proposed Work

Definition of Done

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	pub struct ProviderRefreshConfigInput<'a> {
	pub name: &'a str,
	pub credential_key: &'a str,
	pub strategy: &'a str,
	pub material: &'a [String],
	pub secret_material_keys: &'a [String],
	pub credential_expires_at_ms: Option<i64>,
	}

chore: refactor sandbox_create to use a builder pattern #1408

Description

Description

Context

Proposed Work

Definition of Done

Activity

github-actions commented on Jun 16, 2026

lunarwhite commented on Jun 16, 2026

drew commented on Jun 16, 2026

lunarwhite commented on Jun 17, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions