refactor(cli): replace sandbox_create positional args with SandboxCreateConfig struct#1997
Open
lunarwhite wants to merge 1 commit into
Open
refactor(cli): replace sandbox_create positional args with SandboxCreateConfig struct#1997lunarwhite wants to merge 1 commit into
lunarwhite wants to merge 1 commit into
Conversation
…ateConfig struct Signed-off-by: Yuedong Wu <dwcn22@outlook.com>
elezar
reviewed
Jun 25, 2026
| /// 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> { |
Member
There was a problem hiding this comment.
Should this be marked test-only?
Contributor
Author
There was a problem hiding this comment.
Thanks for reviewing this PR! The file is under tests/ subfolder, which is a separate crate that won't be compiled into the binary. So I assume it's not required.
Member
|
/ok-to-test 83637ad |
elezar
reviewed
Jun 25, 2026
| let tls = test_tls(&server); | ||
| install_fake_ssh(&fake_ssh_dir); | ||
|
|
||
| let cmd = vec!["echo".into(), "OK".into()]; |
Member
There was a problem hiding this comment.
Not that it's too important, but can one specify this inline?
Contributor
Author
There was a problem hiding this comment.
I don't have a preference. Looking at the broader codebase style, not-inline seems to be the more adopted pattern. If you don't have a strong preference on this either, I'll leave it as-is for now. 🙂
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the 21-parameter positional signature of
run::sandbox_createwith aSandboxCreateConfigstruct, following theProviderRefreshConfigInputprecedent established in PR #1349. This removes theclippy::too_many_argumentssuppression and makes call sites self-documenting via named fields and struct update syntax.Related Issue
Closes #1408
Notes for reviewers:
The original issue suggested a builder pattern (
SandboxCreateBuilder::new(...).name(...).create().await?).This PR proposes to use a plain config struct with
Defaultinstead, for two reasons:ProviderRefreshConfigInput(PR feat(providers): add credential refresh foundation #1349, merged after the issue was filed) established a config-struct convention in the CLI crate, while no builder pattern exists incrates/openshell-cli/.sandbox_createis an async side-effecting operation, not an inert value construction. A builder would require ~18 setter methods for the same named-field ergonomics thatSandboxCreateConfig { ..Default::default() }provides for free. The struct approach solves the stated problem (positional args, readability, fragility) whilestaying consistent with the codebase.
Changes
SandboxCreateConfig<'a>struct withDefaultimpl (safe production defaults:keep: false,approval_mode: "manual")sandbox_create()to accept(server, gateway_name, config, tls), keeping infrastructure params positional per existing conventionmain.rsto construct the config structtest_config()helper in integration tests with test-appropriate defaults (keep: true,tty_override: Some(false)) and migrate all 14 call sitesTesting
mise run pre-commitpassesChecklist