fix(sandbox): stop claiming strict mode is isolation; gate clippy in CI - #13
Merged
Merged
Conversation
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 <noreply@archlinux.org>
…e direction
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 <noreply@archlinux.org>
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 <noreply@archlinux.org>
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 <noreply@archlinux.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes: the P0 honesty fix for the sandbox, and the CI gate that stops this
class of thing regressing.
strict mode overclaimed what it does
best_available_mode()falls back tostrictwhen neither bwrap nor firejail ispresent — 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.
/sandboxclaimed 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 the bigger liability: a reviewer who reads the
code after trusting that sentence has found a credibility problem, not just a missing
feature.
What changed:
denylist (strict — no enforcement), labelled that way wherever shown.
/sandboxprints the platform's actual ceiling — on a machine with no backend itsays so, and notes bubblewrap/firejail are Linux-only.
protected they are, rather than burying it in status output.
strict_check's doc now lists the forms that walk past it and states plainly thatdenylists cannot be completed — so nobody "fixes" it by adding literals and
believing the gap is closed.
No 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 worth scoping properly.
CI now gates clippy
Clippy was clean but ungated — nothing stopped it drifting back one PR at a time,
which is exactly the re-surfacing problem.
cargo clippyruns the fullcargo checkpass plus lints, so this replaces the bare
cargo checkrather than adding astep: same build, more coverage, no extra CI time.
cargo testalso now runs--all-features, matching what is verified locally.Verified the gate bites — introducing a
len() == 0comparison turns a warning intoerror: length comparison to zeroand fails the build (exit 101).Tests
5 new tests pinning the honesty guarantees, including one asserting the old overclaim
string cannot return, and one pinning the known bypasses so docs and code cannot drift
apart.
One of those tests was initially wrong:
X="rm -rf /"; $Xis caught, because theliteral appears in the assignment. Indirection only evades when the string is never
spelled out (
A=rm; B=-rf; $A $B /). Corrected rather than weakened.560 tests pass, 0 failures. Clippy clean under
-D warnings.