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

Reject bad uploads before create. Share the upload transfer helper.

Signed-off-by: Eric Curtin <eric.curtin@docker.com>

Author: ericcurtin

crates/openshell-cli/src/main.rs

@@ -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))

crates/openshell-cli/src/run.rs

@@ -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 {
@@ -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 {
@@ -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());
             }
 
@@ -5012,28 +5003,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,
@@ -5045,12 +5024,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(())

crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs

@@ -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();
@@ -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!(
@@ -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",
     );
 }
 
@@ -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

docs/how-it-works/sandboxes/overview.mdx

@@ -894,12 +894,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

skills/openshell-cli/SKILL.md

@@ -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.