From 602e37e8cfec5fc18d6bcc2c68a77721e3cc9a6f Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:12:56 -0700 Subject: [PATCH 1/4] fix(sandbox): stop claiming strict mode is isolation; gate clippy in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things: an honesty fix for the sandbox, and the CI gate that stops any of this regressing. ## strict mode overclaimed what it does `best_available_mode()` falls back to `strict` when neither bwrap nor firejail is present — which is *always* on macOS and Windows, since both are Linux-only. So on two of three supported platforms, "sandbox ENABLED" meant a lowercase substring denylist and nothing else. The UI said otherwise. `/sandbox` claimed strict "blocks fork-bombs, disk overwrites, and other catastrophic patterns", and listed the three modes as peers. For a product sold to enterprise the false claim is a bigger liability than the gap: a reviewer who reads the code after trusting that sentence has found a credibility problem, not just a missing feature. Now: - Modes are split into *isolation* (bwrap, firejail — kernel-enforced) and *best-effort denylist* (strict — no enforcement), and labelled that way wherever they are shown. - `/sandbox` prints the platform's actual ceiling: on a machine with no backend it says so, and says bubblewrap/firejail are Linux-only. - Enabling a non-enforcing mode warns at the moment the user forms a belief about how protected they are, not buried in status output. - `strict_check`'s doc lists the forms that walk past it (flag order, extra whitespace, long flags, command substitution, split indirection) and says plainly that denylists cannot be completed — so nobody "fixes" it by adding literals and believing the gap is closed. No behaviour change to what is or isn't blocked. This is about not telling users they are protected when they are not. Real isolation on macOS/Windows (sandbox-exec, AppContainer) is a separate piece of work. ## CI now gates clippy Clippy was clean but *ungated* — nothing stopped it drifting back one PR at a time, which is exactly the "bugs re-surfacing" problem. `cargo clippy` performs the full `cargo check` pass plus lints, so this replaces the bare `cargo check` rather than adding a step: same build, more coverage, no extra CI time. `cargo test` also now runs `--all-features` to match what is verified locally. Verified the gate bites: introducing a `len() == 0` comparison turns a warning into `error: length comparison to zero` and fails the build (exit 101). Adds 5 tests pinning the honesty guarantees — including one asserting the old overclaim string cannot come back, and one pinning the known bypasses so the docs and the code cannot drift apart. One of those tests was initially wrong: `X="rm -rf /"; $X` *is* caught, because the literal appears in the assignment. Indirection only evades when the string is never spelled out (`A=rm; B=-rf; $A $B /`). Corrected rather than weakened. Suite: 560 passed, 0 failed. Clippy clean under -D warnings. Co-Authored-By: Arch Linux --- .github/workflows/ci.yml | 10 +- src/sandbox.rs | 207 +++++++++++++++++++++++++++++++++++---- src/tui/run.rs | 17 +++- 3 files changed, 212 insertions(+), 22 deletions(-) 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/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/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 => { From 3b7d49dae42ec6af3b00122c706aa4b43291d183 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:25:24 -0700 Subject: [PATCH 2/4] fix: resolve pre-existing clippy lints the new gate exposed; pin prune direction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clippy gate added in the previous commit failed on all three platforms — working exactly as intended. It surfaced 8 pre-existing lints in code this branch never touched, none of which my local run caught. Why local was clean and CI was not: local clippy was 1.94 (2026-03-02) while CI uses `dtolnay/rust-toolchain@stable`, now 1.97. Five months of new lints. "Clippy clean locally" was not a meaningful claim; local toolchain updated so it is. Lints resolved (all mechanical, no behaviour change): - 5x unnecessary_sort_by → sort_by_key(Reverse(..)) in autocommit, glob, tool_search, sdk, session. Same key, same direction, both stable sorts. - 2x manual_checked_div → checked_div().unwrap_or(0) in commands. The `if limit > 0` guard was dead (limit is a non-zero literal) but the intent is preserved rather than deleted. - 1x redundant reference in format! → autofixed in worktree. - 1x unused_mut exposed by the sort refactor. One of those sort sites was in `prune_old_refs`, which decides **which session snapshots get deleted**. Inverting it would delete the newest sessions instead of the oldest — silent, unrecoverable loss of exactly the history a user would reach for with /undo. The existing integration test could not have caught that. It asserts only counts (5 deleted, 10 remain), never identity, and it cannot be strengthened in place: `%(committerdate:unix)` has one-second granularity, so 15 sessions created in a loop all tie and the surviving set is decided by git's output order rather than recency. So the selection is now a separate `select_refs_to_delete`, with unit tests that pin the direction against synthetic distinct timestamps. Verified: inverting the sort fails `keeps_the_newest_and_deletes_the_oldest`. That extraction also documents a real latent bug rather than hiding it — with tied timestamps, prune keeps refs by alphabetical refname, not recency. A test pins the current behaviour so it is visible; fixing it needs a tie-breaker on the ref's own ordering and is filed in the tracker, not smuggled in here. Suite: 564 passed, 0 failed. Gate passes under the CI toolchain. Co-Authored-By: Arch Linux --- src/autocommit.rs | 66 +++++++++++++++++++++++++++++++++++----- src/commands/mod.rs | 4 +-- src/sdk/mod.rs | 2 +- src/session/mod.rs | 2 +- src/tools/glob.rs | 2 +- src/tools/tool_search.rs | 2 +- src/tools/worktree.rs | 2 +- 7 files changed, 66 insertions(+), 14 deletions(-) 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/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/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 From bccf32c67a8f067a59fbb3f6c6f103273a376f7c Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:38:51 -0700 Subject: [PATCH 3/4] fix: resolve the platform-specific clippy lints the gate exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round from the new gate. Ubuntu went green last commit; macOS and Windows each had lints that only exist on their platform — which is precisely why the gate runs on the full matrix rather than one runner. - deeplink.rs (macOS + Windows): `return Ok(())` was the last statement of the `#[cfg(not(linux))]` block, so on non-Linux builds it is a needless return. Split into two per-platform definitions — the idiomatic form, and it removes the lint rather than suppressing it. - settings.rs (Windows): `mut parsed` is only mutated in the unix ownership/permission branch, so `mut` is unused on Windows. `#[cfg_attr(not(unix), allow(unused_mut))]` scopes the allow to the platform that needs it instead of blanket-allowing it everywhere. - hooks.rs (Windows): `cfg_post` is mine — its only caller is the signal-termination test I gated to unix in the previous PR, leaving the helper dead on Windows. Gated to match. Verification note: I could not check the non-Linux paths locally. `cargo clippy --target x86_64-pc-windows-msvc` fails because the tree-sitter crates need a C cross-compiler that is not installed. The deeplink stub was instead verified by temporarily swapping the cfg attributes so it compiles as the active definition on Linux — clean. The other two are single-line and platform-obvious. Everything else rests on CI. Suite: 564 passed, 0 failed. Gate clean on Linux. Co-Authored-By: Arch Linux --- src/deeplink.rs | 13 +++++++------ src/hooks.rs | 3 +++ src/settings.rs | 3 +++ 3 files changed, 13 insertions(+), 6 deletions(-) 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/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; From 0df0d3cf6e07e66ff9cd36638ad33d642e3485a6 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:52:24 -0700 Subject: [PATCH 4/4] test(auth): remove env mutation that raced under the parallel harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI failure. `config_dir_honours_the_env_override` and `no_subprocess_when_no_profile_directory_exists` both called `std::env::set_var("ANTHROPIC_CONFIG_DIR", ...)`. Env is process-global and cargo runs tests in parallel threads, so the two raced each other — one clobbering the other's value mid-assertion. It happened to pass on Linux and macOS and lost the race on Windows. This is self-inflicted: `auth.rs` already has an `AuthEnv` injection seam whose entire purpose is testing resolution without touching process env, and these two tests bypassed it. Fixed by making the check pure — `profile_dir_exists_at(Option<&Path>)` takes the directory instead of reading it — so both tests operate on a tempdir with no global state. The env-reading wrapper is a one-liner and needs no test. No production behaviour change. Suite: 564 passed, 0 failed. Gate clean. Co-Authored-By: Arch Linux --- src/auth.rs | 45 +++++++++++++++++---------------------------- 1 file changed, 17 insertions(+), 28 deletions(-) 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]