From 15f10761305fd5cbbb1d64e541d513d6ca5a3527 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 5 Jun 2026 13:47:51 -0400 Subject: [PATCH 1/2] fix(cli): fall back to regular upload when git filtering excludes all files When uploading a directory whose contents are entirely excluded by .gitignore, git_sync_files returns an empty file list. The git-aware upload path silently accepted this and printed "Upload complete" without transferring any files. Now both call sites (direct CLI handler and sandbox_upload_plan) check for an empty file list and fall through to the regular, unfiltered upload path instead. Closes #1778 Signed-off-by: Russell Bryant --- crates/openshell-cli/src/main.rs | 5 ++++- crates/openshell-cli/src/run.rs | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 21f15834b..440e05ac2 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -2653,7 +2653,10 @@ async fn main() -> Result<()> { } let dest_display = sandbox_dest.unwrap_or("~"); eprintln!("Uploading {} -> sandbox:{}", local.display(), dest_display); - if !no_git_ignore && let Ok((base_dir, files)) = run::git_sync_files(local) { + if !no_git_ignore + && let Ok((base_dir, files)) = run::git_sync_files(local) + && !files.is_empty() + { run::sandbox_sync_up_files( &ctx.endpoint, &name, diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 3b2adfc59..0cd01a3fa 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -5752,6 +5752,7 @@ fn sandbox_upload_plan(local_path: &Path, git_ignore: bool) -> Result Date: Fri, 5 Jun 2026 13:54:19 -0400 Subject: [PATCH 2/2] test(e2e): add regression test for gitignored upload fallback (#1778) Reproduces the reported workflow: download a file from a sandbox into a local git repo where the target directory is gitignored, then re-upload that directory without --no-git-ignore. Before the fix the upload silently succeeded with zero files transferred. Signed-off-by: Russell Bryant --- e2e/rust/tests/sync.rs | 104 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 98 insertions(+), 6 deletions(-) diff --git a/e2e/rust/tests/sync.rs b/e2e/rust/tests/sync.rs index a42447865..a5d6b5a3d 100644 --- a/e2e/rust/tests/sync.rs +++ b/e2e/rust/tests/sync.rs @@ -73,7 +73,10 @@ async fn sandbox_file_upload_download_round_trip() { // Verify nested file. let nested = fs::read_to_string(download_dir.join("subdir/nested.txt")) .expect("read subdir/nested.txt after download"); - assert_eq!(nested, "nested-content", "subdir/nested.txt content mismatch"); + assert_eq!( + nested, "nested-content", + "subdir/nested.txt content mismatch" + ); // --------------------------------------------------------------- // Step 4 — Large-file round-trip (~512 KiB) to exercise multi-chunk @@ -112,7 +115,8 @@ async fn sandbox_file_upload_download_round_trip() { .await .expect("download large file"); - let actual_data = fs::read(large_down.join("large.bin")).expect("read large.bin after download"); + let actual_data = + fs::read(large_down.join("large.bin")).expect("read large.bin after download"); let actual_hash = { let mut hasher = Sha256::new(); hasher.update(&actual_data); @@ -152,8 +156,8 @@ async fn sandbox_file_upload_download_round_trip() { .await .expect("download single file"); - let single_content = fs::read_to_string(single_down.join("single.txt")) - .expect("read single.txt after download"); + let single_content = + fs::read_to_string(single_down.join("single.txt")).expect("read single.txt after download"); assert_eq!( single_content, "single-file-payload", "single.txt content mismatch" @@ -364,7 +368,11 @@ async fn sandbox_download_file_only() { .expect("sandbox create --keep"); guard - .exec(&["sh", "-c", "printf greeting-payload > /sandbox/greeting.txt"]) + .exec(&[ + "sh", + "-c", + "printf greeting-payload > /sandbox/greeting.txt", + ]) .await .expect("seed greeting.txt inside the sandbox"); @@ -436,7 +444,9 @@ async fn sandbox_download_directory_only() { /// match the short markers individually instead. fn assert_resolves_outside_workspace(err: &str, label: &str) { assert!( - err.contains("resolves to") && err.contains("outside the") && err.contains("sandbox workspace"), + err.contains("resolves to") + && err.contains("outside the") + && err.contains("sandbox workspace"), "expected resolves-outside-workspace error for {label}, got: {err}" ); } @@ -543,3 +553,85 @@ async fn sandbox_download_handles_dash_leading_basename() { guard.cleanup().await; } + +/// Regression for #1778: uploading a directory whose contents are entirely +/// excluded by `.gitignore` must still transfer the files instead of +/// silently reporting success with zero bytes sent. +/// +/// Reproduces the reported workflow: download files from a sandbox into a +/// local git repo where the target directory is gitignored, then re-upload +/// to the same sandbox at a different path. +#[tokio::test] +async fn upload_gitignored_directory_falls_back_to_unfiltered() { + let mut guard = + SandboxGuard::create_keep(&["sh", "-c", "echo Ready && sleep infinity"], "Ready") + .await + .expect("sandbox create --keep"); + + // Seed a file inside the sandbox so we have something to download. + guard + .exec(&[ + "sh", + "-c", + "mkdir -p /sandbox/runs && printf downloaded-payload > /sandbox/runs/test.json", + ]) + .await + .expect("seed runs/test.json inside the sandbox"); + + // Download the file into a local git repo where `runs/` is gitignored. + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + let repo = tmpdir.path().join("repo"); + fs::create_dir_all(&repo).expect("create repo dir"); + + let git_init = tokio::process::Command::new("git") + .args(["init"]) + .current_dir(&repo) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .await + .expect("git init"); + assert!(git_init.success(), "git init should succeed"); + + fs::write(repo.join(".gitignore"), "runs/\n").expect("write .gitignore"); + + let download_dest = repo.join("runs"); + fs::create_dir_all(&download_dest).expect("create runs dir"); + let download_str = download_dest.to_str().expect("download path is UTF-8"); + + guard + .download("/sandbox/runs/test.json", download_str) + .await + .expect("download runs/test.json"); + + let local_file = download_dest.join("test.json"); + assert!(local_file.exists(), "downloaded file should exist locally"); + + // Re-upload the gitignored directory (without --no-git-ignore). + let upload_str = download_dest.to_str().expect("upload path is UTF-8"); + guard + .upload_with_gitignore(upload_str, "/sandbox/reuploaded", &repo) + .await + .expect("upload gitignored directory"); + + // Download the re-uploaded file and verify it arrived. + let verify_dir = tmpdir.path().join("verify"); + fs::create_dir_all(&verify_dir).expect("create verify dir"); + let verify_str = verify_dir.to_str().expect("verify path is UTF-8"); + + guard + .download("/sandbox/reuploaded", verify_str) + .await + .expect("download reuploaded directory"); + + // The upload wraps contents under the source basename, so the file + // lands at verify/runs/test.json. + let reuploaded = fs::read_to_string(verify_dir.join("runs/test.json")) + .expect("reuploaded test.json should exist"); + assert_eq!( + reuploaded, "downloaded-payload", + "reuploaded file content mismatch — gitignored directory was not uploaded" + ); + + guard.cleanup().await; +}