feat(clean): add biwa clean command to remove stale remote directories#415
feat(clean): add biwa clean command to remove stale remote directories#415
Conversation
Implements #371. Adds a new `biwa clean` command to manage and remove stale remote project directories, with automatic background cleanup. New features: - `biwa clean` — remove current project's remote directory - `biwa clean --all` — remove all this client's tracked remote dirs - `biwa clean --purge` — remove ALL biwa dirs under remote_root - `biwa clean --dry-run` — preview what would be removed - `biwa clean stop` — stop the background cleanup daemon - Automatic background cleanup after `biwa run`/`biwa sync` - Quota-aware cleanup thresholds (configurable via `clean.quota_thresholds`) - Local connection cache at `$BIWA_CACHE_DIR/connections.json` New modules: - `src/duration.rs` — HumanDuration type for config parsing - `src/cache.rs` — local connection tracking and PID management - `src/ssh/clean.rs` — quota parsing, remote dir listing, rm -rf - `src/cli/clean.rs` — CLI command with subcommands and flags Configuration: - `clean.max_age` — hard age limit (default: 30 days) - `clean.auto` — enable background auto-cleanup (default: true) - `clean.quota_thresholds` — percentage-based cleanup thresholds BREAKING CHANGE: Remote directory naming format changed from `{name}-{combined_hash}` to `{name}-{host_hash}-{path_hash}` to support client identification for cleanup. Existing remote directories will not be recognized; use `biwa clean --purge` to remove old ones.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
biwa-docs | 885008b | Commit Preview URL Branch Preview URL |
Mar 22 2026, 04:19 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
biwa-docs | 9265658 | Commit Preview URL Branch Preview URL |
Mar 24 2026, 05:01 AM |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive remote directory cleanup mechanism for biwa. It provides a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new biwa clean command to manage and remove stale remote project directories, along with automatic background cleanup triggered by biwa run/biwa sync. It includes new modules for duration parsing, connection tracking, and remote directory listing/removal. The remote directory naming format has also been changed. The review focuses on correctness and maintainability, with specific attention to error handling, code duplication, and adherence to general rules.
There was a problem hiding this comment.
Pull request overview
Adds a new biwa clean feature to remove stale remote project directories (manually via CLI and automatically via a background daemon), along with config/schema/docs updates and a revised remote directory naming scheme to support client identification.
Changes:
- Introduces
biwa cleanCLI command (plusstop,--all,--purge,--dry-run) and background auto-cleanup triggered bybiwa run/biwa sync. - Adds local cache tracking (
connections.json) + PID management for the cleanup daemon. - Updates remote project directory naming to
{project}-{host_hash}-{path_hash}and adds duration/quota parsing utilities plus config/schema/docs updates.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/common/mod.rs | Updates test helper to match the new remote dir naming format. |
| src/ssh/sync.rs | Adds client host-hash helper and updates unique project dir naming. |
| src/ssh/mod.rs | Exposes new SSH cleanup module. |
| src/ssh/clean.rs | Adds quota parsing + remote dir listing/removal helpers (with tests). |
| src/main.rs | Registers new cache and duration modules. |
| src/duration.rs | Adds HumanDuration wrapper for config-friendly durations (with tests). |
| src/config/types.rs | Adds [clean] config section and quota-threshold logic. |
| src/config/load.rs | Updates config load tests to include clean defaults. |
| src/cli/mod.rs | Wires new clean subcommand into the CLI. |
| src/cli/clean.rs | Implements biwa clean behavior, including daemon spawning and auto-clean logic. |
| src/cli/run.rs | Records connections and triggers background cleanup after runs. |
| src/cli/sync.rs | Records connections and triggers background cleanup after syncs. |
| src/cache.rs | Implements cache storage and daemon PID lifecycle helpers (with tests). |
| schema/config.json | Updates generated config schema to include clean and HumanDuration. |
| docs/src/cli/index.md | Adds clean links to CLI docs index. |
| docs/src/cli/clean.md | Adds generated CLI docs for biwa clean. |
| docs/src/cli/clean/stop.md | Adds generated CLI docs for biwa clean stop. |
| biwa.usage.kdl | Adds usage spec entries for clean. |
| src/cli/snapshots/* | Updates init-config snapshots to include clean section. |
| Cargo.toml / Cargo.lock | Adds dependencies for duration parsing, timestamps, cache dir discovery, daemonization helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 41.28% 43.41% +2.13%
==========================================
Files 16 24 +8
Lines 671 1868 +1197
==========================================
+ Hits 277 811 +534
- Misses 394 1057 +663 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix CleanConfig.auto schema default (false -> true) via schema_defaults::clean_auto() - Fix cache desync: only remove successfully-deleted entries in --all, --purge, and auto cleanup - Fix is_daemon_running() to treat EPERM as running (only ESRCH means not running) - Regenerate schema/config.json
risu729
force-pushed
the
feat/clean-remote-dirs
branch
from
8f9a8a4 to
d957c41
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
Use $XDG_STATE_HOME/biwa (default ~/.local/state/biwa) with BIWA_STATE_DIR override instead of the cache hierarchy and BIWA_CACHE_DIR. Made-with: Cursor
There was a problem hiding this comment.
Code Review
This pull request introduces a biwa clean command for managing stale remote directories, including an automatic background cleanup mechanism. The implementation adds new dependencies, configuration options, and several new modules for caching, duration parsing, and cleanup logic. A key change is the modification of the remote directory naming scheme to include a client-specific hash, which is a breaking change.
My review focuses on the new logic. I've found a couple of potential issues: one related to a buggy fallback path for the cache directory, and another concerning potential integer truncation when calculating disk quota percentages. I've provided suggestions to address these.
- write_pid_file: return early when a daemon is already running (no clobber) - state_dir: safe cwd fallback instead of a literal ~ path segment - Bounded parallel remote removals via Semaphore (8 concurrent) - Skip background auto-cleanup when ssh.password is interactive-only - Quota usage_percent: use u64→f64 without u32 truncation; add large-value test - list_remote_dirs: use find -- for path safety Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Parse quota block fields with optional * suffix; add regression test - remove_connections_for_target(host, user, port, dirs) to avoid cross-host cache drops - Post-process generated CLI docs: biwa clean [FLAGS] [SUBCOMMAND] (usage-lib quirk) - Keep Clean clap subcommand optional via explicit subcommand_required = false Made-with: Cursor
- Replace src/cache.rs with src/state.rs (mod state) - Rename Cache/CachedConnection to State/Connection - Rename load_cache/save_cache to load_state/save_state - Update CLI imports and user-facing log/comment wording Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a biwa clean command for managing stale remote directories, a valuable feature for disk space management. It includes multiple cleaning strategies, configuration options for automatic cleanup based on age and disk quota, and a background daemon. The implementation is well-structured, with new modules for duration parsing, state management, and cleaning logic. The use of atomic writes for state and scopeguard for PID file cleanup demonstrates robustness. My review includes a critical suggestion to prevent potential deadlocks during daemon spawning and a few medium-severity suggestions to improve code readability and maintainability.
- Replace manual XDG_CONFIG_HOME env reads with dirs (consistent with state.rs) - Rename load_internal parameter to config_dir; update related comments Made-with: Cursor
Only delete default-layout remote dirs (remote_root + host hash) for clean --all and auto; list untracked matching dirs on the remote as stale. Purge behavior unchanged. kill_daemon keeps the PID file unless SIGTERM succeeds or the PID is stale (ESRCH). Reject clean.quota_thresholds keys above 100. Use io::Error::from(Errno) in setsid pre_exec to avoid allocation issues. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Err(e) = fs::remove_file(&path) | ||
| && e.kind() != io::ErrorKind::NotFound | ||
| { | ||
| warn!(error = %e, path = %path.display(), "Failed to remove PID file"); |
There was a problem hiding this comment.
remove_pid_file() uses if let Err(e) = fs::remove_file(&path) && e.kind() != io::ErrorKind::NotFound, which is not valid Rust syntax and won't compile. Capture the Result first (or use a match) and only warn when the error is not NotFound.
| if let Err(e) = fs::remove_file(&path) | |
| && e.kind() != io::ErrorKind::NotFound | |
| { | |
| warn!(error = %e, path = %path.display(), "Failed to remove PID file"); | |
| match fs::remove_file(&path) { | |
| Ok(_) => {} | |
| Err(e) if e.kind() != io::ErrorKind::NotFound => { | |
| warn!(error = %e, path = %path.display(), "Failed to remove PID file"); | |
| } | |
| Err(_) => {} |
| pub fn is_default_biwa_remote_dir(remote_dir: &str, remote_root: &Path, host_hash: &str) -> bool { | ||
| if host_hash.len() != 8 || !host_hash.chars().all(|c| c.is_ascii_hexdigit()) { | ||
| return false; | ||
| } | ||
| if !remote_dir.contains(host_hash) { | ||
| return false; | ||
| } | ||
| let root = remote_root.to_string_lossy(); | ||
| let root = root.trim_end_matches('/'); | ||
| if root.is_empty() { | ||
| return false; | ||
| } | ||
| let prefix = format!("{root}/"); | ||
| let Some(rest) = remote_dir.strip_prefix(&prefix) else { | ||
| return false; | ||
| }; | ||
| !rest.is_empty() && !rest.contains('/') && !rest.contains("..") |
There was a problem hiding this comment.
is_default_biwa_remote_dir() only checks that remote_dir contains host_hash somewhere and is a direct child of remote_root. This can yield false positives (e.g., project names containing that 8-hex substring, or extra - segments), which weakens the safety guard meant to prevent deleting the wrong directories. Consider validating the basename against the expected {name}-{host_hash}-{path_hash} layout: split on - from the end and require the second-to-last segment == host_hash and the last segment is exactly 8 hex chars.
| for key in &[ | ||
| "BIWA_SSH_HOST", | ||
| "BIWA_SSH_PORT", | ||
| "BIWA_SSH_USER", | ||
| "BIWA_SSH_PASSWORD", | ||
| ] { | ||
| if let Ok(val) = env::var(key) { | ||
| cmd.env(key, val); | ||
| } | ||
| } |
There was a problem hiding this comment.
spawn_background_cleanup() claims it forwards SSH config via environment so the daemon connects to the same host even if run from a different working directory, but it currently only forwards values if those env vars were already set. If the parent connection info came from a config file (common), the child may load a different config (or fail required config checks) and connect to the wrong host. Set BIWA_SSH_HOST/PORT/USER (and non-interactive password if applicable) from config.ssh explicitly rather than conditionally reading env::var().
| for key in &[ | |
| "BIWA_SSH_HOST", | |
| "BIWA_SSH_PORT", | |
| "BIWA_SSH_USER", | |
| "BIWA_SSH_PASSWORD", | |
| ] { | |
| if let Ok(val) = env::var(key) { | |
| cmd.env(key, val); | |
| } | |
| } | |
| // | |
| // Use the resolved config.ssh values as the source of truth so that the | |
| // child process does not depend on its own working directory or config. | |
| let ssh_cfg = &config.ssh; | |
| cmd.env("BIWA_SSH_HOST", &ssh_cfg.host); | |
| cmd.env("BIWA_SSH_PORT", ssh_cfg.port.to_string()); | |
| if let Some(user) = &ssh_cfg.user { | |
| cmd.env("BIWA_SSH_USER", user); | |
| } | |
| // Preserve any existing non-interactive password passed via the environment. | |
| if let Ok(val) = env::var("BIWA_SSH_PASSWORD") { | |
| cmd.env("BIWA_SSH_PASSWORD", val); | |
| } |
Disable background auto-clean in shared e2e helpers, add explicit clean target selection tests, and cover clean command behavior with new e2e cases. Document automatic and manual cleanup behavior in sync docs so flag precedence and safety expectations are clear.
Add clean.state_dir to config and resolve state directory in Config with precedence BIWA_STATE_DIR > config value > XDG default. Refactor state APIs to take explicit state paths, thread the resolved path through clean/run/sync flows, and add load tests plus regenerated schema/snapshots. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Err(e) = fs::remove_file(&path) | ||
| && e.kind() != io::ErrorKind::NotFound | ||
| { | ||
| warn!(error = %e, path = %path.display(), "Failed to remove PID file"); |
There was a problem hiding this comment.
remove_pid_file has a compile-time logic error: if let Err(e) = fs::remove_file(&path) && e.kind() != ... is not valid Rust and also won’t correctly filter out NotFound. Restructure this to first call fs::remove_file and then, inside the Err(e) branch, ignore io::ErrorKind::NotFound and warn on any other error.
| if let Err(e) = fs::remove_file(&path) | |
| && e.kind() != io::ErrorKind::NotFound | |
| { | |
| warn!(error = %e, path = %path.display(), "Failed to remove PID file"); | |
| if let Err(e) = fs::remove_file(&path) { | |
| if e.kind() != io::ErrorKind::NotFound { | |
| warn!(error = %e, path = %path.display(), "Failed to remove PID file"); | |
| } |
| pub fn is_default_biwa_remote_dir(remote_dir: &str, remote_root: &Path, host_hash: &str) -> bool { | ||
| if host_hash.len() != 8 || !host_hash.chars().all(|c| c.is_ascii_hexdigit()) { | ||
| return false; | ||
| } | ||
| if !remote_dir.contains(host_hash) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
is_default_biwa_remote_dir currently only checks remote_dir.contains(host_hash), which can produce false positives (e.g., any unrelated directory name under remote_root that happens to contain the same 8-hex substring). Since this guard is used to decide what’s safe to delete in clean --auto/--all, consider validating the final path component matches the expected default layout more strictly (e.g., ends with -{host_hash}-<8 hex> and contains no extra path separators).
Place state_dir on Config instead of clean; resolve it in resolve_paths_partial. Update load tests and regenerate schema/init snapshots. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -5,6 +5,12 @@ expression: content | |||
| { | |||
| "$schema": "https://biwa.takuk.me/schema/config.json", | |||
| { | |||
There was a problem hiding this comment.
The generated jsonc init template content is not valid JSONC because it starts an object twice ({ on line 5 and another { on line 7). This likely comes from prepending $schema to a template that already contains its own opening brace; adjust the init generation logic so $schema is inserted into the same object (and regenerate the snapshots accordingly).
| { |
| @@ -5,6 +5,12 @@ expression: content | |||
| { | |||
| $schema: "https://biwa.takuk.me/schema/config.json", | |||
| { | |||
There was a problem hiding this comment.
The generated json5 init template content is not valid JSON5 because it starts an object twice ({ on line 5 and another { on line 7). This suggests the init generator is prepending $schema without removing the template’s own opening brace; update the generator to merge them into a single object and then regenerate this snapshot.
| { |
Closes #371
Summary
Adds a new
biwa cleancommand to manage and remove stale remote project directories, with automatic background cleanup triggered bybiwa run/biwa sync.New Commands
biwa clean— remove current project's remote directorybiwa clean --all— remove all this client's tracked remote dirsbiwa clean --purge— remove ALL biwa dirs underremote_rootbiwa clean --dry-run— preview what would be removedbiwa clean stop— stop the background cleanup daemonConfiguration
Implementation
New Modules
src/duration.rs—HumanDurationtype for human-readable duration configsrc/state.rs— local connection tracking (connections.jsonunder XDG state) + PID managementsrc/ssh/clean.rs— quota parsing (quota -w), remote dir listing,rm -rfsrc/cli/clean.rs— CLI command with subcommands and flagsNew Dependencies
humantime— duration parsingchrono— timestamp serializationdirs— XDG state directory resolutionnix—setsid()for daemon detachscopeguard— PID file cleanup guardBreaking Change
Remote directory naming format changed from
{name}-{combined_hash}to{name}-{host_hash}-{path_hash}to support client identification for cleanup. Existing remote directories will not be recognized; usebiwa clean --purgeto remove old ones.