fix: skip session upload from an empty directory or $HOME - #1113
Conversation
Activating a session ran the non-VCS-root upload confirmation for any directory that is not a recognized VCS root, with no regard for whether it was empty or the user's home. From an empty directory the prompt asked about uploading nothing; from $HOME a single Enter would bulk-upload the whole home tree, and when $HOME was itself a VCS root the upload ran with no prompt at all. Add an is_empty_or_home predicate and, before the non-VCS-root gate, skip the upload without prompting for an empty directory or $HOME -- even when $HOME holds files or is a VCS root -- printing "Starting with an empty box (nothing here to sync)" to stderr while the session id stays the only stdout line. An explicit --sync tarball restores the upload as an escape hatch; to tell it apart from the default, --sync now records whether it was passed.
📝 WalkthroughWalkthroughActivation now detects empty directories and ChangesWorkspace upload suppression
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimal/src/lib.rs`:
- Around line 1427-1430: Update the activation flow around skip_empty_or_home so
the synchronous file_upload::is_empty_or_home inspection runs inside
tokio::task::spawn_blocking rather than on the async runtime thread. Preserve
the existing condition and arguments, await the blocking task, and propagate any
task join error through the surrounding result path.
In `@crates/minimal/tests/cli.rs`:
- Line 224: Extend the integration fixtures in crates/minimal/tests/cli.rs to
cover implicit sync behavior with sync: None, including the default upload path
and skipped empty/$HOME roots. Assert the expected workspace and stderr
contracts for each case, while retaining existing explicit SyncMode::Tarball
coverage; run the relevant integration test command afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f977076c-03e6-4557-8155-34cfcea6e972
📒 Files selected for processing (3)
crates/minimal/src/file_upload.rscrates/minimal/src/lib.rscrates/minimal/tests/cli.rs
| let skip_empty_or_home = !sync_explicit | ||
| && upload_root.as_ref().is_some_and(|root| { | ||
| file_upload::is_empty_or_home(root.as_std_path(), std::env::home_dir().as_deref()) | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move directory inspection off the async runtime thread.
is_empty_or_home performs synchronous canonicalize and read_dir here. A stalled filesystem can block the Tokio worker during activation; run this check in tokio::task::spawn_blocking and propagate the join error.
Proposed fix
- let skip_empty_or_home = !sync_explicit
- && upload_root.as_ref().is_some_and(|root| {
- file_upload::is_empty_or_home(root.as_std_path(), std::env::home_dir().as_deref())
- });
+ let skip_empty_or_home = if !sync_explicit {
+ if let Some(root) = upload_root.as_ref() {
+ let root = root.clone();
+ let home = std::env::home_dir();
+ tokio::task::spawn_blocking(move || {
+ file_upload::is_empty_or_home(root.as_std_path(), home.as_deref())
+ })
+ .await
+ .context("checking whether upload directory is empty or home")?
+ } else {
+ false
+ }
+ } else {
+ false
+ };Based on learnings, avoid direct std::fs work from async tasks; use spawn_blocking.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let skip_empty_or_home = !sync_explicit | |
| && upload_root.as_ref().is_some_and(|root| { | |
| file_upload::is_empty_or_home(root.as_std_path(), std::env::home_dir().as_deref()) | |
| }); | |
| let skip_empty_or_home = if !sync_explicit { | |
| if let Some(root) = upload_root.as_ref() { | |
| let root = root.clone(); | |
| let home = std::env::home_dir(); | |
| tokio::task::spawn_blocking(move || { | |
| file_upload::is_empty_or_home(root.as_std_path(), home.as_deref()) | |
| }) | |
| .await | |
| .context("checking whether upload directory is empty or home")? | |
| } else { | |
| false | |
| } | |
| } else { | |
| false | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimal/src/lib.rs` around lines 1427 - 1430, Update the activation
flow around skip_empty_or_home so the synchronous file_upload::is_empty_or_home
inspection runs inside tokio::task::spawn_blocking rather than on the async
runtime thread. Preserve the existing condition and arguments, await the
blocking task, and propagate any task join error through the surrounding result
path.
Source: Learnings
| name: Some("test-session".to_string()), | ||
| path: Some(project.path().to_string_lossy().to_string()), | ||
| sync: SyncMode::Tarball, | ||
| sync: Some(SyncMode::Tarball), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add integration coverage for implicit sync behavior.
These fixtures all use Some(SyncMode::Tarball), so they only exercise the explicit escape hatch. Add activation coverage using sync: None for the default upload path and for skipped empty/$HOME roots, asserting the workspace and stderr contract.
As per coding guidelines, “When changing VM or daemon behavior, add or update the appropriate integration tests and run just e2e and/or just test-vm.”
Also applies to: 266-266, 345-345
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimal/tests/cli.rs` at line 224, Extend the integration fixtures in
crates/minimal/tests/cli.rs to cover implicit sync behavior with sync: None,
including the default upload path and skipped empty/$HOME roots. Assert the
expected workspace and stderr contracts for each case, while retaining existing
explicit SyncMode::Tarball coverage; run the relevant integration test command
afterward.
Source: Coding guidelines
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABK34wfQ
min session activatestreams the project directory into the new session's workspace, but the upload gate only checked whether the directory was a VCS root. From an empty directory it prompted to upload nothing; from$HOMEa single Enter uploaded the whole home tree; and a$HOMEthat was itself a VCS root uploaded with no prompt at all.This adds an
is_empty_or_homepredicate and, ahead of the non-VCS-root confirmation, skips the upload without prompting for an empty directory or$HOME— even when$HOMEholds files or is a VCS root — printingStarting with an empty box (nothing here to sync)to stderr while the session id stays the only stdout line. An explicit--sync tarballstill uploads them as the escape hatch;--syncnow records whether it was passed so the deliberate flag can be told from the default.Verification
cargo fmt --all --check --manifest-path target/Cargo.toml— clean, no driftcargo clippy --workspace --locked --manifest-path target/Cargo.toml -- -D warnings— 0 warningscargo build --workspace --locked --manifest-path target/Cargo.toml— Finished (exit 0)cargo test --workspace --locked --manifest-path target/Cargo.toml— all tests passed, 0 failed; the 4 newis_empty_or_homeunit tests pass (4 passed; 0 failed)Note
Skip session upload when the target directory is empty or is
$HOMEis_empty_or_homeinfile_upload.rsthat returnstrueif a directory is the user's home directory or is empty (checked via canonical path comparison andread_dir).cmd_activate, the tarball upload is skipped when the resolved upload root is empty or equals$HOME, printing "Starting with an empty box (nothing here to sync)".--sync tarballexplicitly bypasses this check and forces the upload regardless.ActivateArgs.syncchanges fromSyncModetoOption<SyncMode>; absence defaults toTarballin code rather than via clap.Macroscope summarized b8ced5a.
Summary by CodeRabbit
New Features
Bug Fixes