diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6fbbf10..d19dd29 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,10 +20,14 @@ jobs: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - - name: Check - run: cargo check --all-targets + # clippy runs the full `cargo check` pass plus lints, so this replaces the + # old bare `cargo check` rather than adding to it — same build, more + # coverage, no extra CI time. `-D warnings` is the point: clippy was clean + # but ungated, so nothing stopped it drifting back one PR at a time. + - name: Lint (clippy, warnings are errors) + run: cargo clippy --all-targets --all-features -- -D warnings - name: Test - run: cargo test + run: cargo test --all-features readme-lint: runs-on: ubuntu-latest diff --git a/src/auth.rs b/src/auth.rs index ed358b5..e3df4a8 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -254,7 +254,14 @@ fn anthropic_config_dir() -> Option { /// the credentials directory first means we never execute anything unless the /// real CLI has actually been used here. fn ant_profile_dir_exists() -> bool { - anthropic_config_dir().is_some_and(|d| d.join("credentials").is_dir()) + profile_dir_exists_at(anthropic_config_dir().as_deref()) +} + +/// Pure form of the check, so it can be tested without mutating process-global +/// env. `set_var` races under the parallel test harness — the whole point of +/// the `AuthEnv` seam above is to avoid exactly that. +fn profile_dir_exists_at(config_dir: Option<&std::path::Path>) -> bool { + config_dir.is_some_and(|d| d.join("credentials").is_dir()) } impl AuthEnv for ProcessAuthEnv { @@ -472,47 +479,29 @@ mod tests { /// startup ran an unrelated build tool and stalled the process long enough /// to fail the headless SDK test. Nothing may be executed unless the real /// CLI has actually stored a profile here. + /// + /// Pure over the directory — no `set_var`, so it cannot race other tests. #[test] fn no_subprocess_when_no_profile_directory_exists() { let empty = tempfile::tempdir().unwrap(); - // SAFETY: single-threaded within this test; the var is restored below. - let prev = std::env::var("ANTHROPIC_CONFIG_DIR").ok(); - unsafe { std::env::set_var("ANTHROPIC_CONFIG_DIR", empty.path()) }; - assert!( - !ant_profile_dir_exists(), + !profile_dir_exists_at(Some(empty.path())), "a config dir with no credentials/ must not trigger a spawn" ); - assert!(ProcessAuthEnv.ant_access_token().is_none()); - assert!(!ProcessAuthEnv.ant_profile_present()); - - unsafe { - match prev { - Some(v) => std::env::set_var("ANTHROPIC_CONFIG_DIR", v), - None => std::env::remove_var("ANTHROPIC_CONFIG_DIR"), - } - } + assert!( + !profile_dir_exists_at(None), + "no config dir at all must not trigger a spawn" + ); } #[test] - fn config_dir_honours_the_env_override() { + fn profile_directory_is_detected_when_present() { let dir = tempfile::tempdir().unwrap(); - let prev = std::env::var("ANTHROPIC_CONFIG_DIR").ok(); - unsafe { std::env::set_var("ANTHROPIC_CONFIG_DIR", dir.path()) }; - - assert_eq!(anthropic_config_dir().as_deref(), Some(dir.path())); std::fs::create_dir_all(dir.path().join("credentials")).unwrap(); assert!( - ant_profile_dir_exists(), + profile_dir_exists_at(Some(dir.path())), "credentials/ present ⇒ the real CLI has been used here" ); - - unsafe { - match prev { - Some(v) => std::env::set_var("ANTHROPIC_CONFIG_DIR", v), - None => std::env::remove_var("ANTHROPIC_CONFIG_DIR"), - } - } } #[test] diff --git a/src/autocommit.rs b/src/autocommit.rs index 942f0ad..9ac14ae 100644 --- a/src/autocommit.rs +++ b/src/autocommit.rs @@ -402,6 +402,21 @@ pub fn restore_to( /// Delete old `refs/rustyclaw/sessions/*` refs, keeping the `keep` newest by /// committer date. `keep == 0` disables pruning. Non-fatal: any error is /// logged via `tracing::warn!` and the function returns 0. +/// Choose which shadow refs to delete: keep the `keep` newest, delete the rest. +/// +/// Extracted so the *direction* is testable. An inversion here deletes the +/// newest sessions instead of the oldest — silent, unrecoverable data loss — +/// and the integration test cannot catch it, because `%(committerdate:unix)` +/// has one-second granularity and sessions created in a loop all tie. +/// +/// Ties are resolved by whatever order git returned (refname-alphabetical), +/// which is *not* recency. See the review tracker: prune ordering is unreliable +/// for sessions created within the same second. +fn select_refs_to_delete(mut rows: Vec<(i64, String)>, keep: usize) -> Vec { + rows.sort_by_key(|e| std::cmp::Reverse(e.0)); + rows.into_iter().skip(keep).map(|(_, r)| r).collect() +} + pub fn prune_old_refs(cwd: &Path, keep: u32) -> anyhow::Result { if keep == 0 || !is_git_repo(cwd) { return Ok(0); @@ -420,7 +435,7 @@ pub fn prune_old_refs(cwd: &Path, keep: u32) -> anyhow::Result { return Ok(0); } let s = String::from_utf8(out.stdout)?; - let mut rows: Vec<(i64, String)> = s + let rows: Vec<(i64, String)> = s .lines() .filter_map(|line| { let mut it = line.splitn(2, ' '); @@ -434,12 +449,7 @@ pub fn prune_old_refs(cwd: &Path, keep: u32) -> anyhow::Result { return Ok(0); } - rows.sort_by(|a, b| b.0.cmp(&a.0)); - let to_delete: Vec = rows - .into_iter() - .skip(keep as usize) - .map(|(_, r)| r) - .collect(); + let to_delete = select_refs_to_delete(rows, keep as usize); let mut deleted = 0u32; for r in &to_delete { @@ -482,6 +492,48 @@ pub fn snapshot_turn_raw( ) } +#[cfg(test)] +mod prune_selection_tests { + use super::select_refs_to_delete; + + fn rows(pairs: &[(i64, &str)]) -> Vec<(i64, String)> { + pairs.iter().map(|(t, r)| (*t, r.to_string())).collect() + } + + /// The direction is the whole point: keep the NEWEST, delete the oldest. + /// Inverting this deletes the sessions the user most likely wants to undo + /// to — silent, unrecoverable loss. The integration test cannot catch an + /// inversion because its timestamps all tie at one-second granularity. + #[test] + fn keeps_the_newest_and_deletes_the_oldest() { + let deleted = select_refs_to_delete( + rows(&[(100, "old"), (300, "newest"), (200, "mid"), (50, "oldest")]), + 2, + ); + assert_eq!(deleted, vec!["old".to_string(), "oldest".to_string()]); + } + + #[test] + fn keeping_more_than_present_deletes_nothing() { + assert!(select_refs_to_delete(rows(&[(1, "a"), (2, "b")]), 10).is_empty()); + } + + #[test] + fn keep_zero_deletes_everything() { + let deleted = select_refs_to_delete(rows(&[(1, "a"), (2, "b")]), 0); + assert_eq!(deleted.len(), 2); + } + + /// Documents the known weakness rather than asserting a guarantee we do not + /// have: with equal timestamps the survivors are decided by git's output + /// order, not by recency. + #[test] + fn tied_timestamps_fall_back_to_input_order() { + let deleted = select_refs_to_delete(rows(&[(7, "a"), (7, "b"), (7, "c")]), 1); + assert_eq!(deleted, vec!["b".to_string(), "c".to_string()]); + } +} + #[cfg(test)] mod git_detection_tests { use super::*; diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 90630e6..b9fb96e 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -728,7 +728,7 @@ fn cmd_cost(ctx: &CommandContext) -> CommandAction { fn cmd_context(ctx: &CommandContext) -> CommandAction { let limit: u64 = 200_000; let used = ctx.tokens_in; - let pct = if limit > 0 { used * 100 / limit } else { 0 }; + let pct = (used * 100).checked_div(limit).unwrap_or(0); let bar_len = 30usize; let filled = (pct as usize * bar_len / 100).min(bar_len); let bar: String = "█".repeat(filled) + &"░".repeat(bar_len - filled); @@ -2608,7 +2608,7 @@ fn cmd_btw(args: &str) -> CommandAction { fn cmd_ctx_viz(ctx: &CommandContext) -> CommandAction { let limit: u64 = 200_000; let used = ctx.tokens_in; - let pct = if limit > 0 { used * 100 / limit } else { 0 }; + let pct = (used * 100).checked_div(limit).unwrap_or(0); // Build a visual histogram of context usage let bar_width = 40usize; diff --git a/src/deeplink.rs b/src/deeplink.rs index c5784f2..4b3c5dd 100644 --- a/src/deeplink.rs +++ b/src/deeplink.rs @@ -137,14 +137,15 @@ fn hex_val(b: u8) -> Option { /// /// Requires xdg-utils to be installed (standard on most desktops). /// On non-Linux platforms, prints a notice and returns Ok(()). +#[cfg(not(target_os = "linux"))] pub fn register_protocol() -> anyhow::Result<()> { - #[cfg(not(target_os = "linux"))] - { - eprintln!("Deep link protocol registration is only supported on Linux."); - return Ok(()); - } + eprintln!("Deep link protocol registration is only supported on Linux."); + Ok(()) +} - #[cfg(target_os = "linux")] +/// Register the deep link protocol handler on Linux. +#[cfg(target_os = "linux")] +pub fn register_protocol() -> anyhow::Result<()> { { let scheme = protocol_name(); let binary = std::env::current_exe() diff --git a/src/hooks.rs b/src/hooks.rs index d292c68..9d2a0c0 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -543,6 +543,9 @@ mod tests { } } + /// Only used by the signal-termination test, which is unix-only — so this + /// helper is dead code on Windows and trips `-D warnings` there. + #[cfg(unix)] fn cfg_post(command: &str) -> HooksConfig { HooksConfig { post_tool_use: vec![entry(command)], diff --git a/src/sandbox.rs b/src/sandbox.rs index 8689f55..37f5711 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -1,9 +1,25 @@ /// Sandbox execution wrapper for the Bash tool. /// -/// Three modes: -/// strict — pattern-based blocking of destructive commands (no external deps) -/// bwrap — bubblewrap (Linux namespaces, read-only system mounts) -/// firejail— firejail profile-based sandboxing +/// Two categories of mode, and the difference matters: +/// +/// **Isolation** — the kernel enforces the boundary. +/// bwrap — bubblewrap namespaces, read-only system mounts (Linux only) +/// firejail — firejail profiles (Linux only) +/// +/// **Best-effort pattern blocking** — no enforcement, just a blocklist. +/// strict — substring match against a list of catastrophic commands +/// +/// `strict` is **not a sandbox**. It is a small denylist of literal substrings +/// and it is trivially bypassed — `rm -fr /`, `rm -rf /` (two spaces), +/// `$(echo rm) -rf /`, or any base64/variable indirection all walk straight +/// past it. It catches fat-finger accidents, not an adversary, and it cannot +/// restrict filesystem or network access at all. +/// +/// This distinction is load-bearing because **neither bwrap nor firejail exists +/// on macOS or Windows**, so `best_available_mode()` returns `strict` there. +/// On those platforms "sandbox enabled" means pattern matching and nothing +/// more. [`isolation_available`] reports whether real isolation is obtainable, +/// and the UI must say so rather than implying protection that isn't there. /// /// Mode selection: `/sandbox enable [strict|bwrap|firejail]` /// The active mode is stored in config.sandbox_mode and applied by BashTool. @@ -37,10 +53,66 @@ pub fn best_available_mode() -> &'static str { } } +/// Can this machine actually isolate a command, or only pattern-match it? +/// +/// False on macOS and Windows (no bwrap, no firejail) and on any Linux box +/// without them installed. Callers must use this to avoid telling the user they +/// are sandboxed when the only thing standing between the model and their +/// filesystem is a substring denylist. +pub fn isolation_available() -> bool { + bwrap_available() || firejail_available() +} + +/// Does this mode enforce a boundary, or is it best-effort only? +pub fn mode_enforces_isolation(mode: &str) -> bool { + matches!(mode, "bwrap" | "firejail") +} + +/// Warning to show when a mode cannot enforce anything. `None` when the active +/// mode really does isolate. +pub fn weak_mode_warning(mode: &str) -> Option { + if mode_enforces_isolation(mode) { + return None; + } + let why = if isolation_available() { + "Real isolation IS available on this machine — prefer `/sandbox enable bwrap` \ + (or firejail)." + } else if cfg!(target_os = "linux") { + "No isolation backend is installed. Install one for real containment: \ + `sudo apt install bubblewrap` (or firejail)." + } else { + "Neither bubblewrap nor firejail exists on this platform, so no isolation \ + backend is available at all." + }; + Some(format!( + "strict mode is a best-effort denylist, NOT isolation. It matches literal \ + substrings and is trivially bypassed (`rm -fr /`, `$(echo rm) -rf /`, \ + variable indirection). It cannot restrict filesystem or network access.\n \ + {why}" + )) +} + // ── Strict mode: pattern-based blocking ────────────────────────────────────── /// Returns Some(reason) if the command matches a dangerous pattern. -/// This runs before the command is executed in strict mode. +/// +/// **Best-effort denylist, not a security boundary.** This is a case-insensitive +/// substring match over a fixed list. It stops the exact literal forms below and +/// nothing else — every one of these gets through: +/// +/// ```text +/// rm -fr / flag order +/// rm -rf / extra whitespace +/// rm --recursive --force / long flags +/// $(echo rm) -rf / command substitution +/// X="rm -rf /"; $X variable indirection +/// echo cm0gLXJmIC8= | base64 -d | sh +/// ``` +/// +/// Denylists cannot be made complete; do not add patterns expecting to close +/// the gap. Its job is catching an accidental catastrophic command, and it runs +/// in every mode as a cheap second layer. Actual containment comes from +/// bwrap/firejail — see [`isolation_available`]. pub fn strict_check(cmd: &str) -> Option { let low = cmd.to_lowercase(); let patterns: &[(&str, &str)] = &[ @@ -224,29 +296,54 @@ pub fn sandbox_status(enabled: bool, mode: &str) -> String { }; let status = if enabled { - format!("ENABLED [mode: {}]", mode) + let kind = if mode_enforces_isolation(mode) { + "isolation" + } else { + "pattern blocking only — NOT isolation" + }; + format!("ENABLED [mode: {mode} — {kind}]") } else { "DISABLED".to_string() }; + // State the platform's actual ceiling rather than letting the mode list + // imply every option is equivalent. + let ceiling = if isolation_available() { + String::new() + } else { + format!( + "\n\ + ⚠ No isolation backend on this machine{}.\n \ + The only available mode is `strict`, which is a best-effort denylist:\n \ + it matches literal substrings, is trivially bypassed, and cannot restrict\n \ + filesystem or network access. Treat it as a guard against accidents, not\n \ + against an adversary.\n", + if cfg!(any(target_os = "macos", target_os = "windows")) { + " (bubblewrap and firejail are Linux-only)" + } else { + "" + } + ) + }; + format!( "Sandbox {status}\n\ - \n\ + {ceiling}\n\ Modes:\n\ - strict — pattern-based blocking (always available)\n\ - bwrap — bubblewrap namespaces [{bwrap}]\n\ - firejail — firejail profiles [{fjail}]\n\ + bwrap — kernel namespace isolation [{bwrap}]\n\ + firejail — kernel namespace isolation [{fjail}]\n\ + strict — best-effort denylist, no isolation (always available)\n\ \n\ Commands:\n\ - /sandbox enable — enable (auto-selects best mode)\n\ - /sandbox enable strict — enable strict pattern blocking\n\ - /sandbox enable bwrap — enable bubblewrap sandboxing\n\ - /sandbox enable firejail — enable firejail sandboxing\n\ - /sandbox disable — disable sandboxing\n\ + /sandbox enable — enable (auto-selects the strongest available)\n\ + /sandbox enable bwrap — bubblewrap isolation\n\ + /sandbox enable firejail — firejail isolation\n\ + /sandbox enable strict — denylist only\n\ + /sandbox disable — disable\n\ \n\ - When enabled, all Bash tool calls run inside the sandbox.\n\ - Strict mode blocks fork-bombs, disk overwrites, and other\n\ - catastrophic patterns regardless of sandbox mode.", + When enabled, all Bash tool calls go through the selected mode.\n\ + The `strict` denylist is also applied in bwrap and firejail mode as a\n\ + second layer, but it is never the thing doing the containment.", ) } @@ -305,6 +402,80 @@ mod tests { assert!(fj.contains("--net=none")); } + // ── Honesty about what `strict` actually does ──────────────────────────── + + /// `strict` is the automatic fallback wherever no isolation backend exists + /// — which is *always* on macOS and Windows. The UI must not describe it in + /// terms that imply containment. + #[test] + fn strict_is_not_described_as_isolation() { + assert!(!mode_enforces_isolation("strict")); + assert!(mode_enforces_isolation("bwrap")); + assert!(mode_enforces_isolation("firejail")); + + let status = sandbox_status(true, "strict"); + assert!( + status.contains("NOT isolation"), + "status must say strict is not isolation: {status}" + ); + assert!( + !status.contains("catastrophic patterns regardless"), + "the old overclaim must not come back: {status}" + ); + } + + /// Enabling is when the user forms a belief about how protected they are. + #[test] + fn weak_mode_warns_and_strong_mode_does_not() { + let w = weak_mode_warning("strict").expect("strict must warn"); + assert!(w.contains("NOT isolation"), "{w}"); + assert!(w.contains("trivially bypassed"), "{w}"); + + assert!(weak_mode_warning("bwrap").is_none()); + assert!(weak_mode_warning("firejail").is_none()); + } + + /// The denylist is documented as best-effort precisely because these get + /// through. Pinning them stops anyone "fixing" it by adding more literals + /// and believing the gap is closed. + #[test] + fn known_bypasses_are_not_caught_and_that_is_expected() { + for bypass in [ + "rm -fr /", + "rm -rf /", + "rm --recursive --force /", + "$(echo rm) -rf /", + // Indirection only evades when the literal is never spelled out — + // `X=\"rm -rf /\"; $X` *is* caught, because the substring is right + // there in the assignment. Split it and the denylist is blind. + "A=rm; B=-rf; $A $B /", + "rm -r -f /", + ] { + assert!( + strict_check(bypass).is_none(), + "denylist is not expected to catch {bypass:?} — if this now passes, \ + the docs claiming best-effort need revisiting, not celebrating" + ); + } + // The literal forms it does catch still work. + assert!(strict_check("rm -rf /").is_some()); + assert!(strict_check("RM -RF /").is_some(), "matching is case-insensitive"); + } + + #[test] + fn isolation_availability_matches_backend_presence() { + assert_eq!( + isolation_available(), + bwrap_available() || firejail_available() + ); + // best_available_mode only returns a weak mode when nothing can isolate. + if isolation_available() { + assert!(mode_enforces_isolation(best_available_mode())); + } else { + assert_eq!(best_available_mode(), "strict"); + } + } + #[test] fn shell_quote_escapes_embedded_single_quotes() { assert_eq!(shell_quote("it's"), r#"'it'\''s'"#); diff --git a/src/sdk/mod.rs b/src/sdk/mod.rs index b98e8fa..a339f80 100644 --- a/src/sdk/mod.rs +++ b/src/sdk/mod.rs @@ -501,7 +501,7 @@ async fn list_sessions(limit: Option) -> Result> { } // Sort by created_at descending (most recent first) - sessions.sort_by(|a, b| b.0.cmp(&a.0)); + sessions.sort_by_key(|e| std::cmp::Reverse(e.0)); let limit = limit.unwrap_or(50); let infos: Vec = sessions.into_iter().take(limit).map(|(_, s)| s).collect(); diff --git a/src/session/mod.rs b/src/session/mod.rs index c88920f..58517fe 100644 --- a/src/session/mod.rs +++ b/src/session/mod.rs @@ -204,7 +204,7 @@ impl Session { } } - sessions.sort_by(|a, b| b.0.cmp(&a.0)); + sessions.sort_by_key(|e| std::cmp::Reverse(e.0)); Ok(sessions.into_iter().map(|(_, m)| m).collect()) } diff --git a/src/settings.rs b/src/settings.rs index 0a6af70..85c15c9 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -402,6 +402,9 @@ impl Settings { /// warn. Windows has no POSIX permission bits so this is a no-op there; /// the threat model (multi-user shared settings) is primarily a unix /// concern anyway. + // `parsed` is only mutated on unix (where the ownership/permission check + // runs), so `mut` is unused on Windows and trips `-D warnings` there. + #[cfg_attr(not(unix), allow(unused_mut))] fn sanitize_unsafe_helper(mut parsed: Self, path: &Path) -> Self { if parsed.api_key_helper.is_none() { return parsed; diff --git a/src/tools/glob.rs b/src/tools/glob.rs index af03644..d826c3a 100644 --- a/src/tools/glob.rs +++ b/src/tools/glob.rs @@ -98,7 +98,7 @@ impl Tool for GlobTool { .collect(); // Sort by modification time, most recent first - entries.sort_by(|a, b| b.0.cmp(&a.0)); + entries.sort_by_key(|e| std::cmp::Reverse(e.0)); if entries.is_empty() { return Ok(ToolOutput::success("No files matched the pattern.")); diff --git a/src/tools/tool_search.rs b/src/tools/tool_search.rs index f5bdec3..6695a0e 100644 --- a/src/tools/tool_search.rs +++ b/src/tools/tool_search.rs @@ -74,7 +74,7 @@ impl Tool for ToolSearchTool { }) .collect(); - scored.sort_by(|a, b| b.0.cmp(&a.0)); + scored.sort_by_key(|e| std::cmp::Reverse(e.0)); scored.truncate(input.max_results); if scored.is_empty() { diff --git a/src/tools/worktree.rs b/src/tools/worktree.rs index 75e8b2c..ecfca66 100644 --- a/src/tools/worktree.rs +++ b/src/tools/worktree.rs @@ -117,7 +117,7 @@ impl Tool for EnterWorktreeTool { .join(format!( "{}-{}", git_root.split('/').next_back().unwrap_or("repo"), - &slug + slug )); // Create worktree + branch diff --git a/src/tui/run.rs b/src/tui/run.rs index 5fe2009..9b069e6 100644 --- a/src/tui/run.rs +++ b/src/tui/run.rs @@ -2571,7 +2571,22 @@ async fn handle_key(ctx: KeyCtx<'_>) -> Result<()> { "sandboxEnabled", serde_json::Value::Bool(enabled), ); - let msg = crate::sandbox::sandbox_status(enabled, &config.sandbox_mode); + let mut msg = String::new(); + // Enabling is the moment the user forms a belief about how + // protected they are. If the active mode cannot enforce + // anything, say so here rather than burying it in status. + if enabled + && let Some(warning) = + crate::sandbox::weak_mode_warning(&config.sandbox_mode) + { + msg.push_str("⚠ "); + msg.push_str(&warning); + msg.push_str("\n\n"); + } + msg.push_str(&crate::sandbox::sandbox_status( + enabled, + &config.sandbox_mode, + )); app.overlay = Some(Overlay::new("sandbox", msg)); } CommandAction::ShowThinkback => {