Skip to content

fix(cli): plan sandbox uploads before provisioning #4193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
johntmyers merged 1 commit into from
Oct 6, 2026
Merged

fix(cli): plan sandbox uploads before provisioning #4193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions crates/openshell-cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3414,14 +3414,6 @@ async fn run_async() -> Result<()> {
})
.collect();

// Validate all local paths before creating the sandbox so failures are
// fast and have no side effects.
for (local, _, _) in &upload_specs {
if std::fs::symlink_metadata(local).is_err() {
return Err(miette::miette!("local path does not exist: {}", local));
}
}

let editor = editor.map(Into::into);
let forward = forward
.map(|s| openshell_core::forward::ForwardSpec::parse(&s))
Expand Down
104 changes: 58 additions & 46 deletions crates/openshell-cli/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,12 @@ pub async fn sandbox_create(
return Err(miette::miette!("--expose port must be in 1..=65535"));
}

// Plan every upload before provisioning so a rejected one leaves no sandbox.
let upload_plans = uploads
.iter()
.map(|(local_path, _, git_ignore)| sandbox_upload_plan(Path::new(local_path), *git_ignore))
.collect::<Result<Vec<_>>>()?;

// Check port availability *before* creating the sandbox so we don't
// leave an orphaned sandbox behind when the forward would fail.
if let Some(ref spec) = forward {
Expand Down Expand Up @@ -1033,7 +1039,9 @@ pub async fn sandbox_create(
drop(client);

let upload_count = uploads.len();
for (idx, (local_path, sandbox_path, git_ignore)) in uploads.iter().enumerate() {
for (idx, ((local_path, sandbox_path, _), upload_plan)) in
uploads.iter().zip(upload_plans).enumerate()
{
let dest = sandbox_path.as_deref();
let dest_display = dest.unwrap_or("~");
if upload_count > 1 {
Expand All @@ -1049,38 +1057,21 @@ pub async fn sandbox_create(
"\u{2022}".dimmed(),
);
}
let local = Path::new(local_path);
let upload_plan = sandbox_upload_plan(local, *git_ignore).wrap_err_with(|| {
sandbox_upload_planned(
upload_plan,
&effective_server,
&sandbox_name,
Path::new(local_path),
dest,
&effective_tls,
workspace,
)
.await
.wrap_err_with(|| {
format!(
"Sandbox '{sandbox_name}' was created and still exists.\nRetry the upload with 'openshell sandbox upload', or remove the sandbox with 'openshell sandbox delete'",
)
})?;
match upload_plan {
SandboxUploadPlan::GitAware { base_dir, files } => {
sandbox_sync_up_files(
&effective_server,
&sandbox_name,
&base_dir,
&files,
local,
dest,
&effective_tls,
workspace,
)
.await?;
}
SandboxUploadPlan::Regular => {
sandbox_sync_up(
&effective_server,
&sandbox_name,
local,
dest,
&effective_tls,
workspace,
)
.await?;
}
}
eprintln!(" {} Files uploaded", "\u{2713}".green().bold());
}

Expand Down Expand Up @@ -4977,28 +4968,16 @@ fn git_filtered_upload_plan(local_path: &Path) -> Result<SandboxUploadPlan> {
Ok(SandboxUploadPlan::GitAware { base_dir, files })
}

/// Upload a local path to a sandbox.
///
/// Symlink sources, including dangling links, bypass Git-aware filtering so
/// the tar upload preserves the link instead of dereferencing its target.
pub async fn sandbox_upload(
async fn sandbox_upload_planned(
plan: SandboxUploadPlan,
server: &str,
name: &str,
local_path: &Path,
sandbox_path: Option<&str>,
git_ignore: bool,
tls: &TlsOptions,
workspace: &str,
) -> Result<()> {
let upload_plan = sandbox_upload_plan(local_path, git_ignore)?;
let dest_display = sandbox_path.unwrap_or("~");
eprintln!(
"Uploading {} -> sandbox:{}",
local_path.display(),
dest_display
);

match upload_plan {
match plan {
SandboxUploadPlan::GitAware { base_dir, files } => {
sandbox_sync_up_files(
server,
Expand All @@ -5010,12 +4989,45 @@ pub async fn sandbox_upload(
tls,
workspace,
)
.await?;
.await
}
SandboxUploadPlan::Regular => {
sandbox_sync_up(server, name, local_path, sandbox_path, tls, workspace).await?;
sandbox_sync_up(server, name, local_path, sandbox_path, tls, workspace).await
}
}
}

/// Upload a local path to a sandbox.
///
/// Symlink sources, including dangling links, bypass Git-aware filtering so
/// the tar upload preserves the link instead of dereferencing its target.
pub async fn sandbox_upload(
server: &str,
name: &str,
local_path: &Path,
sandbox_path: Option<&str>,
git_ignore: bool,
tls: &TlsOptions,
workspace: &str,
) -> Result<()> {
let upload_plan = sandbox_upload_plan(local_path, git_ignore)?;
let dest_display = sandbox_path.unwrap_or("~");
eprintln!(
"Uploading {} -> sandbox:{}",
local_path.display(),
dest_display
);

sandbox_upload_planned(
upload_plan,
server,
name,
local_path,
sandbox_path,
tls,
workspace,
)
.await?;

eprintln!("{} Upload complete", "✓".green().bold());
Ok(())
Expand Down
52 changes: 38 additions & 14 deletions crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3172,12 +3172,14 @@ async fn run_cli_sandbox_create(
}

#[tokio::test]
async fn sandbox_create_upload_stops_before_ssh_when_git_filtering_fails_or_is_empty() {
async fn sandbox_create_upload_is_rejected_before_provisioning_when_planning_fails() {
let server = run_server().await;
let source = tempfile::tempdir().unwrap();
fs::create_dir(source.path().join("runs")).unwrap();
fs::write(source.path().join("runs/marker.txt"), "dummy content").unwrap();
fs::write(source.path().join(".gitignore"), "runs/\n").unwrap();
let plain = tempfile::tempdir().unwrap();
fs::write(plain.path().join("ok.txt"), "ok").unwrap();

// A broken repository must not be mistaken for a non-repository source.
fs::create_dir(source.path().join(".git")).unwrap();
Expand All @@ -3188,12 +3190,7 @@ async fn sandbox_create_upload_stops_before_ssh_when_git_filtering_fails_or_is_e
assert!(!result.status.success(), "{stderr}");
assert!(stderr.contains("Git filtering failed"), "{stderr}");
assert!(stderr.contains("--no-git-ignore"), "{stderr}");
assert!(
stderr.contains("Sandbox 'upload-no-repository' was created and still exists"),
"{stderr}",
);
assert!(stderr.contains("openshell sandbox upload"), "{stderr}");
assert!(stderr.contains("openshell sandbox delete"), "{stderr}");
assert!(!stderr.contains("was created"), "{stderr}");

fs::remove_dir(source.path().join(".git")).unwrap();
assert!(
Expand All @@ -3213,21 +3210,41 @@ async fn sandbox_create_upload_stops_before_ssh_when_git_filtering_fails_or_is_e
"{stderr}"
);
assert!(stderr.contains("--no-git-ignore"), "{stderr}");
assert!(
stderr.contains("Sandbox 'upload-empty-selection' was created and still exists"),
"{stderr}",
assert!(!stderr.contains("was created"), "{stderr}");

// A valid earlier upload must not get through when a later one is rejected.
let args = [
"--detach",
"--upload",
plain.path().to_str().unwrap(),
"--upload",
path.to_str().unwrap(),
];
let result = run_cli_sandbox_create(&server, "upload-later-rejected", &args).await;
let stderr = String::from_utf8_lossy(&result.stderr);
assert!(!result.status.success(), "{stderr}");
assert!(stderr.contains("filtering selected no files"), "{stderr}");

let missing = plain.path().join("missing");
let args = ["--detach", "--upload", missing.to_str().unwrap()];
let result = run_cli_sandbox_create(&server, "upload-missing", &args).await;
let stderr = String::from_utf8_lossy(&result.stderr);
assert!(!result.status.success(), "{stderr}");
assert!(stderr.contains("local path does not exist"), "{stderr}");

assert_eq!(
create_requests(&server).await.len(),
0,
"a rejected upload plan must not provision a sandbox",
);
// Upload rejection intentionally leaves the provisioned sandbox available
// for an explicit retry; it does not roll back sandbox creation.
assert_eq!(create_requests(&server).await.len(), 2);
assert_eq!(
server
.openshell
.state
.ssh_session_requests
.load(Ordering::SeqCst),
0,
"a rejected creation-time upload must not open an SSH session",
"a rejected upload plan must not open an SSH session",
);
}

Expand All @@ -3251,6 +3268,13 @@ async fn sandbox_create_upload_warns_and_reaches_ssh_outside_git_repository() {
"{stderr}"
);
assert!(!stderr.contains("Git filtering failed"), "{stderr}");
// A transfer failure after provisioning keeps the sandbox and says so.
assert!(
stderr.contains("Sandbox 'upload-non-repository' was created and still exists"),
"{stderr}"
);
assert!(stderr.contains("openshell sandbox upload"), "{stderr}");
assert!(stderr.contains("openshell sandbox delete"), "{stderr}");
assert_eq!(create_requests(&server).await.len(), 1);
assert!(
server
Expand Down
13 changes: 7 additions & 6 deletions docs/how-it-works/sandboxes/overview.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,12 +871,13 @@ repository stops the upload. Pass `--no-git-ignore` to intentionally upload
without filtering. These rules apply to both `sandbox upload` and
`sandbox create --upload`.

`sandbox create --upload` provisions the sandbox before checking Git filtering.
If filtering rejects an upload, the command exits with an error, but the sandbox
remains running. Earlier uploads in the same command may already have completed.
Use `openshell sandbox upload` to retry against the existing sandbox, or
`openshell sandbox delete` to remove it. Add `--no-git-ignore` to the retry only
if you intend to upload without filtering.
`sandbox create --upload` checks every upload locally before it provisions the
sandbox. If any upload is rejected, including a missing path or a Git filtering
failure, the command exits with an error and creates no sandbox. If a transfer
fails after provisioning, the sandbox remains running and earlier uploads may
have completed. Use `openshell sandbox upload` to retry against the existing
sandbox, or `openshell sandbox delete` to remove it. Add `--no-git-ignore` to the
retry only if you intend to upload without filtering.

Uploads preserve symlinks, including dangling links, instead of dereferencing
their targets. A symlink source bypasses Git filtering so the link itself is
Expand Down
2 changes: 1 addition & 1 deletion skills/openshell-cli/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ openshell sandbox download my-sandbox output ./local-output

Uploads inside a Git work tree honor `.gitignore` by default and stop if Git filtering fails or selects no files. Sources confirmed to be outside a Git work tree upload without filtering, with a warning that `.gitignore` rules are not applied. Git must be available to determine whether filtering applies; broken or inaccessible repositories stop the upload. Add `--no-git-ignore` for an intentional unfiltered upload. This also applies to `sandbox create --upload`.

If `sandbox create --upload` rejects an upload, the sandbox remains running and earlier uploads may have completed. Retry with `sandbox upload` against that sandbox, or remove it with `sandbox delete`.
`sandbox create --upload` checks every upload locally before provisioning and creates no sandbox if one is rejected. If a transfer fails after provisioning, the sandbox remains running and earlier uploads may have completed. Retry with `sandbox upload` against that sandbox, or remove it with `sandbox delete`.

Uploads preserve symlinks, including dangling symlinks, instead of dereferencing their targets. A symlink source bypasses Git-aware filtering so the link itself is archived.

Expand Down
Loading