feat(minimal): warn and confirm before uploading from non-VCS directories - #790
Conversation
…ries When activating a session from a directory that isn't a VCS root, prompt the user for confirmation before the recursive file upload to prevent accidentally uploading large trees like a home directory. Detection covers Git, Mercurial, SVN, CVS, and Jujutsu. Non-interactive contexts (CI, pipes, agents) skip the prompt and proceed. Closes #770.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTarball activation now resolves the project root, detects common VCS markers, and conditionally uploads workspace files. Non-VCS roots prompt for confirmation interactively, while non-interactive execution proceeds without prompting. Malformed project files cause activation to fail. ChangesVCS-aware workspace upload
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_activate
participant resolve_upload_root
participant file_upload_is_vcs_root
participant InteractiveConfirmation
participant upload_workspace_files
cmd_activate->>resolve_upload_root: resolve project root
resolve_upload_root-->>cmd_activate: upload root or error
cmd_activate->>file_upload_is_vcs_root: check upload root
file_upload_is_vcs_root-->>cmd_activate: VCS result
alt non-VCS root and interactive
cmd_activate->>InteractiveConfirmation: request upload confirmation
InteractiveConfirmation-->>cmd_activate: confirmation result
end
alt upload permitted
cmd_activate->>upload_workspace_files: upload resolved root
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
| let should_upload = file_upload::is_vcs_root(utf8_path.as_std_path()) | ||
| || !std::io::stdin().is_terminal() | ||
| || confirm(&format!( | ||
| "{utf8_path} is not a version control repository root. \ |
There was a problem hiding this comment.
is the utf8_path one of:
- the CWD with a minimal.toml / .minimal/minimal.toml
- if above doesn't hold, an ancestor dir that has one of those minimal.toml files
- if none of the above hold the CWD
Ie it reflects the previous CLIs search alg?
There was a problem hiding this comment.
This is a good point, we probably want to walk the tree and always do the upload/thing from the root of the project.
For doing that for the mfile you can run mfile::File::from_dir_recursive then you can call repo_path() to get the base. https://github.com/gominimal/minimal/blob/main/crates/mfile/src/lib.rs#L652
Tarball sync uploaded from whatever directory the user ran `minimal activate` in, so activating from a subdir uploaded only the subdir. Resolve the upload root the same way the CLI discovers config — walking up to the nearest minimal.toml — and use that for both the VCS check and the upload. Falls back to the input path when no config is found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1002-1009: The upload guard’s file_upload::is_vcs_root check
accepts nested CVS directories based only on marker presence, bypassing
confirmation. Update is_vcs_root to validate an actual checkout root rather than
any nested VCS metadata directory, and add coverage for nested CVS directories
while preserving recognized root behavior.
🪄 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: 5d8b2ad3-7fff-406c-84dc-de6cbf6ef6bc
📒 Files selected for processing (1)
crates/minimal/src/lib.rs
| // Guard against accidentally uploading a non-VCS directory | ||
| // (e.g. `~`): if the resolved project root is not a recognized | ||
| // VCS root, warn and ask for confirmation before the recursive | ||
| // upload. On non-interactive stdin (CI, pipes, agents) we | ||
| // proceed without prompting — `--sync none` remains available | ||
| // for explicit opt-out (#770). | ||
| let should_upload = file_upload::is_vcs_root(upload_root.as_std_path()) | ||
| || !std::io::stdin().is_terminal() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Validate actual VCS roots, not just marker presence.
At Line 1008, a nested CVS working directory is treated as a repository root because it contains CVS/. With no mfile, that bypasses this PR’s confirmation despite uploading from a non-root directory. Make the detector VCS-aware enough to distinguish checkout roots, and cover nested CVS directories.
🤖 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 1002 - 1009, The upload guard’s
file_upload::is_vcs_root check accepts nested CVS directories based only on
marker presence, bypassing confirmation. Update is_vcs_root to validate an
actual checkout root rather than any nested VCS metadata directory, and add
coverage for nested CVS directories while preserving recognized root behavior.
…ad-confirm-770 # Conflicts: # crates/minimal/src/lib.rs
resolve_upload_root caught every from_dir_recursive error, so a malformed minimal.toml in an ancestor silently fell back to the invocation subdir. The daemon never saw the broken file, fabricated a default config, and activation proceeded with the wrong setup. Only fall back on NotFound; propagate parse and I/O errors.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimal/src/lib.rs (1)
1076-1094: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winResolve and confirm before creating the session.
CreateSessionhas already succeeded when these new fallible calls run. A malformed ancestor config or stdin read error returns through?without requesting session cleanup, leaving a created but unconfigured session. Compute the upload root and confirmation decision before connecting to the daemon; upload only after an ID exists.🤖 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 1076 - 1094, Move the fallible upload-root resolution and confirmation logic currently using resolve_upload_root and confirm before the CreateSession/daemon connection flow. Preserve the VCS-root check and non-interactive behavior, then create the session only after validation succeeds and perform the upload once the session ID exists, ensuring errors before session creation cannot leave an unconfigured session.
🧹 Nitpick comments (1)
crates/minimal/src/lib.rs (1)
2136-2144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the malformed ancestor case.
The contract is specifically about a broken config found while walking upward. Resolve from a nested child directory so the test catches errors being swallowed during traversal.
Proposed test adjustment
std::fs::write(dir.path().join(mfile::MFILE_NAME), "not valid toml = =").unwrap(); - let path = camino::Utf8Path::from_path(dir.path()).expect("temp path is UTF-8"); - assert!(resolve_upload_root(path).is_err()); + let root = camino::Utf8Path::from_path(dir.path()).expect("temp path is UTF-8"); + let child = root.join("nested"); + std::fs::create_dir_all(&child).unwrap(); + assert!(resolve_upload_root(&child).is_err());🤖 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 2136 - 2144, Update resolve_upload_root_errors_on_malformed_mfile to create a nested child directory beneath the directory containing the malformed mfile, then call resolve_upload_root with that child path. Keep the existing assertion that resolution returns an error, ensuring the upward traversal propagates the malformed ancestor configuration failure.
🤖 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 1086-1094: Update the should_upload decision to use
can_prompt_interactively() instead of checking only whether stdin is a terminal,
while preserving the VCS-root shortcut and confirmation behavior. This ensures
confirm is called only when the prompt can be visibly displayed, matching the
later prompt policy.
- Line 1076: Update the async flow around resolve_upload_root so its synchronous
recursive filesystem work runs inside tokio::task::spawn_blocking rather than on
the Tokio worker. Await the blocking task, propagate both filesystem and
task-join errors appropriately, and preserve the resolved upload_root behavior.
---
Outside diff comments:
In `@crates/minimal/src/lib.rs`:
- Around line 1076-1094: Move the fallible upload-root resolution and
confirmation logic currently using resolve_upload_root and confirm before the
CreateSession/daemon connection flow. Preserve the VCS-root check and
non-interactive behavior, then create the session only after validation succeeds
and perform the upload once the session ID exists, ensuring errors before
session creation cannot leave an unconfigured session.
---
Nitpick comments:
In `@crates/minimal/src/lib.rs`:
- Around line 2136-2144: Update resolve_upload_root_errors_on_malformed_mfile to
create a nested child directory beneath the directory containing the malformed
mfile, then call resolve_upload_root with that child path. Keep the existing
assertion that resolution returns an error, ensuring the upward traversal
propagates the malformed ancestor configuration failure.
🪄 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: 368271af-800e-45ef-9011-068aa65212a5
📒 Files selected for processing (1)
crates/minimal/src/lib.rs
| // `minimal activate ./subdir` still uploads the whole | ||
| // project. Falls back to `utf8_path` when no mfile is found | ||
| // anywhere up the tree (#770). | ||
| let upload_root = resolve_upload_root(&utf8_path)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that this crate already enables Tokio's blocking-task support.
fd '^Cargo\.toml$' crates/minimal -x sed -n '1,220p' {}
rg -n -C2 '\bspawn_blocking\s*\(' crates/minimal/srcRepository: gominimal/minimal
Length of output: 3272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant areas in crates/minimal/src/lib.rs
ast-grep outline crates/minimal/src/lib.rs --view expanded >/tmp/minimal_lib_outline.txt
sed -n '1,220p' /tmp/minimal_lib_outline.txt
# Show the call site around the reported line and the helper definition.
rg -n -C 8 'resolve_upload_root|cmd_activate|upload_root' crates/minimal/src/lib.rs
# If the helper is in another file, locate it.
rg -n 'fn resolve_upload_root|resolve_upload_root\(' crates/minimal/srcRepository: gominimal/minimal
Length of output: 16656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how the Tokio runtime is created and whether cmd_activate runs on a worker thread.
rg -n -C 3 '#\[tokio::main|tokio::runtime::Builder|run\(cli\)|cmd_activate\(' crates/minimal/src
# Show the main entrypoint for more context.
sed -n '1,140p' crates/minimal/src/main.rs
# Show the beginning of run() and run_command() to understand execution context.
sed -n '470,520p' crates/minimal/src/lib.rsRepository: gominimal/minimal
Length of output: 6000
Move resolve_upload_root off the async worker crates/minimal/src/lib.rs:1076 still does a synchronous recursive filesystem walk here; wrap it in tokio::task::spawn_blocking so a slow or wedged filesystem can't block the Tokio runtime.
🤖 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` at line 1076, Update the async flow around
resolve_upload_root so its synchronous recursive filesystem work runs inside
tokio::task::spawn_blocking rather than on the Tokio worker. Await the blocking
task, propagate both filesystem and task-join errors appropriately, and preserve
the resolved upload_root behavior.
Source: Learnings
resolve_upload_root ran after CreateSession, so a malformed mfile in an ancestor errored out without aborting the daemon-side session. Move the call before the daemon connection so it fails before any session is created.
confirm() writes the prompt to stderr, so checking only stdin leaves the command waiting on an invisible prompt when stderr is redirected. Use can_prompt_interactively() — already used by the policy prompt path — which checks both.
Summary
When activating a session from a directory that is not a version-control repository root, the CLI now warns the user and asks for confirmation before the recursive file upload. This prevents accidentally uploading large directory trees (e.g. a home directory) when running
minimal activatefrom the wrong place.is_vcs_root(&Path) -> boolto detect Git, Mercurial, SVN, CVS, and Jujutsu repository roots (including Git worktree.gitfiles)cmd_activate, if the project path is not a VCS root and stdin is a TTY, prompts the user before uploading; declining skips the upload and starts the session with an empty workspace--sync noneremains available for explicit opt-outCloses #770.
Summary by CodeRabbit
Note
Warn and prompt before uploading from non-VCS directories in
cmd_activateis_vcs_rootin file_upload.rs to detect VCS roots by checking for.git,.hg,.svn,CVS, and.jjmarkers (supports.gitas file or directory).resolve_upload_rootin lib.rs to walk up the directory tree and find the nearestminimal.toml, using its repo root as the upload directory instead of the invocation path.--synctarball mode,cmd_activatenow warns the user and prompts for confirmation when the upload root is not a VCS root; on decline, the upload is skipped and the workspace starts empty.minimal.tomlis found above it.Changes since #790 opened
resolve_upload_rootfunction to returnResult<Utf8PathBuf, anyhow::Error>and propagate non-NotFound mfile errors [b2bbefd]cmd_activateasync function to handleresolve_upload_rootResult and modified confirmation prompt default [b2bbefd]resolve_upload_rootto accommodateResultreturn type and error expectations [b2bbefd]cmd_activateto execute before daemon connection establishment [f64bbd9]cmd_activateasync function from!std::io::stdin().is_terminal()to!can_prompt_interactively()when determining whether to skip user confirmation prompts before uploading files from non-VCS root directories [b739b94]📊 Macroscope summarized c07be15. 2 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.