mac-storage-cleaner: v2.0.0 hardening update - #792
Conversation
Never-tier enforced mechanically (subtree denials for Photos/backups/ Keychains/Mail/SSH keys, wired into every deletion loop), --dry-run, user whitelist, fail-closed process guards, keep-N retention, three-stage reversible Trash chain, expanded coverage (Android/iOS tooling, Electron caches, Handoff buffers), honest accounting. 68-test suite upstream.
github-actions
Bot
added
the
review-pending
👋 Thanks for contributing, @JubaKitiashvili!This PR touches What happens next
While you wait
This is an automated message. No action is required from you right now — a maintainer will review soon. |
Greptile SummaryThe update substantially expands and hardens the macOS storage-cleaning skill, including preview-by-default cleanup, protected-path validation, retention policies, audit logging, process guards, and reversible Trash handling.
Confidence Score: 4/5The PR is not yet safe to merge because the outstanding process guard can still mistake an unrelated process for the application that owns a cache and skip required cleanup. The cleaner still searches the complete process snapshot for the raw Application Support directory name, so a generic name such as Files Needing Attention: cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/clean-safe.sh
|
| Filename | Overview |
|---|---|
| cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/clean-safe.sh | Adds preview-by-default cleanup and multiple guarded deletion flows, but the previously reported unscoped substring process check remains. |
| cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/lib.sh | Adds protected-path validation, portable version retention, whitelist handling, process helpers, logging rotation, and reversible Trash fallbacks; the previously reported retention defects are fixed for the documented environment. |
| cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/trash-items.sh | Adds strict argument handling, dry-run behavior, audit-log enforcement, blast-radius limits, path validation, and explicit result reporting. |
| cli-tool/components/skills/productivity/mac-storage-cleaner/SKILL.md | Documents preview-first operation, expanded safety guarantees, cross-agent script discovery, configuration, and cleanup behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
U[User or agent invokes cleaner] --> P{Preview or apply?}
P -->|Preview| G[Run whitelist, path, age, and process guards]
P -->|Apply| L{Audit log writable?}
L -->|No| X[Refuse destructive run]
L -->|Yes| G
G --> R{Target permitted and idle?}
R -->|No| S[Skip or refuse]
R -->|Yes| D[Remove safe cache or move item to Trash]
D --> A[Record outcome and reclaimed size]
Reviews (9): Last reviewed commit: "mac-storage-cleaner: v3.0.1 — fix octal ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if command -v pgrep >/dev/null 2>&1; then | ||
| if pgrep -x "$app" >/dev/null 2>&1; then guard_rc=0 | ||
| else guard_rc=$?; [ "$guard_rc" -eq 1 ] || guard_rc=2; fi | ||
| else | ||
| guard_rc=2 | ||
| fi | ||
| if [ "$guard_rc" != 1 ]; then |
There was a problem hiding this comment.
When a running app's process name differs in spelling or capitalization from its Application Support directory, pgrep -x "$app" returns 1 and the cleanup treats the app as idle, deleting cache directories while they are in use and potentially disrupting the app or leaving partially rewritten cache state.
|
@davila7 ready for review 🙏 — and a heads-up on the red X's, same story as #721:
A privileged re-run ("approve and run") should turn SkillSpector green; the Security Audit result reflects repo-wide state rather than this PR. Context for the diff size (+1012/−66): v2.0.0 is a safety-hardening release — the "never delete" tier (Photos libraries, iOS backups, Keychains, Mail/Messages, |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/find-extras.sh">
<violation number="1" location="cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/find-extras.sh:61">
P2: When `Current` cannot be resolved, this report marks every framework version as removable, allowing the user to trash the browser's active framework and break the app. Treat an unresolved target as unknown and skip that framework (or mark all versions unsafe) instead of defaulting to `none`.</violation>
</file>
<file name="cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/lib.sh">
<violation number="1" location="cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/lib.sh:78">
P2: Keep-N can delete one of the newest real DeviceSupport versions when a symlinked directory is present: this test counts the symlink, but `clean-safe.sh` later skips emitted symlinks. Excluding symlinks before incrementing `i` keeps N real versions and avoids an unnecessary re-download.</violation>
<violation number="2" location="cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/lib.sh:239">
P2: The deny validator only exact-matches the system-user roots (`/system`, `/library`, `/usr`, `/usr/local`, `/var`, `...`), while subtree denials are applied only to the MobileSync/Keychains/Mail/Messages/`.ssh`/`.aws`/`.gnupg`/Photos list. A path strictly below a system root (e.g. `/System/Library/...`, `/usr/local/...`, `/var/folders/...`) is therefore not refused, and the ancestor-symlink re-check uses the same equality-only list, so a `$HOME` symlink pointing into `/System`/`/Users`/etc. can smuggle a deeper path past the string checks yet not be caught on resolution. That undercuts the "Never tier mechanically refused" guarantee this PR advertises. Consider denying these roots as subtrees (and any ancestor-resolved path beneath them) rather than exact leaves.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| cur=$(readlink "$fw/Current" 2>/dev/null) | ||
| cur=$(basename "${cur:-none}") |
There was a problem hiding this comment.
P2: When Current cannot be resolved, this report marks every framework version as removable, allowing the user to trash the browser's active framework and break the app. Treat an unresolved target as unknown and skip that framework (or mark all versions unsafe) instead of defaulting to none.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/find-extras.sh, line 61:
<comment>When `Current` cannot be resolved, this report marks every framework version as removable, allowing the user to trash the browser's active framework and break the app. Treat an unresolved target as unknown and skip that framework (or mark all versions unsafe) instead of defaulting to `none`.</comment>
<file context>
@@ -50,6 +50,30 @@ for d in "$HOME/Library/Containers/"*; do
+ "/Applications/Microsoft Edge.app/Contents/Frameworks/Microsoft Edge Framework.framework/Versions" \
+ "/Applications/Brave Browser.app/Contents/Frameworks/Brave Browser Framework.framework/Versions"; do
+ [ -d "$fw" ] || continue
+ cur=$(readlink "$fw/Current" 2>/dev/null)
+ cur=$(basename "${cur:-none}")
+ for v in "$fw"/*; do
</file context>
| cur=$(readlink "$fw/Current" 2>/dev/null) | |
| cur=$(basename "${cur:-none}") | |
| cur=$(readlink "$fw/Current" 2>/dev/null) || { | |
| echo " (Current link unreadable; skipping this framework)" | |
| continue | |
| } | |
| [ -n "$cur" ] || { | |
| echo " (Current link empty; skipping this framework)" | |
| continue | |
| } | |
| cur=$(basename "$cur") | |
| [ -d "$fw/$cur" ] || { | |
| echo " (Current target unavailable; skipping this framework)" | |
| continue | |
| } |
| i=0 | ||
| while IFS= read -r child; do | ||
| [ -n "$child" ] || continue | ||
| [ -d "$dir/$child" ] || continue |
There was a problem hiding this comment.
P2: Keep-N can delete one of the newest real DeviceSupport versions when a symlinked directory is present: this test counts the symlink, but clean-safe.sh later skips emitted symlinks. Excluding symlinks before incrementing i keeps N real versions and avoids an unnecessary re-download.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/lib.sh, line 78:
<comment>Keep-N can delete one of the newest real DeviceSupport versions when a symlinked directory is present: this test counts the symlink, but `clean-safe.sh` later skips emitted symlinks. Excluding symlinks before incrementing `i` keeps N real versions and avoids an unnecessary re-download.</comment>
<file context>
@@ -40,8 +41,48 @@ SAFE_PATHS=(
+ i=0
+ while IFS= read -r child; do
+ [ -n "$child" ] || continue
+ [ -d "$dir/$child" ] || continue
+ i=$((i + 1))
+ [ "$i" -gt "$n" ] && printf '%s\n' "$child"
</file context>
| [ -d "$dir/$child" ] || continue | |
| [ -d "$dir/$child" ] && [ ! -L "$dir/$child" ] || continue |
| # disabling every home-relative deny rule while the /users/<name> carve-out | ||
| # in the case statement below still grants access underneath it. | ||
| while [ "${home_lower%/}" != "$home_lower" ]; do home_lower="${home_lower%/}"; done | ||
| for r in / /system /library /applications /usr /usr/local /bin /sbin /etc \ |
There was a problem hiding this comment.
P2: The deny validator only exact-matches the system-user roots (/system, /library, /usr, /usr/local, /var, ...), while subtree denials are applied only to the MobileSync/Keychains/Mail/Messages/.ssh/.aws/.gnupg/Photos list. A path strictly below a system root (e.g. /System/Library/..., /usr/local/..., /var/folders/...) is therefore not refused, and the ancestor-symlink re-check uses the same equality-only list, so a $HOME symlink pointing into /System//Users/etc. can smuggle a deeper path past the string checks yet not be caught on resolution. That undercuts the "Never tier mechanically refused" guarantee this PR advertises. Consider denying these roots as subtrees (and any ancestor-resolved path beneath them) rather than exact leaves.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli-tool/components/skills/productivity/mac-storage-cleaner/scripts/lib.sh, line 239:
<comment>The deny validator only exact-matches the system-user roots (`/system`, `/library`, `/usr`, `/usr/local`, `/var`, `...`), while subtree denials are applied only to the MobileSync/Keychains/Mail/Messages/`.ssh`/`.aws`/`.gnupg`/Photos list. A path strictly below a system root (e.g. `/System/Library/...`, `/usr/local/...`, `/var/folders/...`) is therefore not refused, and the ancestor-symlink re-check uses the same equality-only list, so a `$HOME` symlink pointing into `/System`/`/Users`/etc. can smuggle a deeper path past the string checks yet not be caught on resolution. That undercuts the "Never tier mechanically refused" guarantee this PR advertises. Consider denying these roots as subtrees (and any ancestor-resolved path beneath them) rather than exact leaves.</comment>
<file context>
@@ -106,33 +155,198 @@ human_kb () {
+ # disabling every home-relative deny rule while the /users/<name> carve-out
+ # in the case statement below still grants access underneath it.
+ while [ "${home_lower%/}" != "$home_lower" ]; do home_lower="${home_lower%/}"; done
+ for r in / /system /library /applications /usr /usr/local /bin /sbin /etc \
+ /var /private /opt /opt/homebrew /users /volumes /dev /tmp \
+ "$home_lower" "$home_lower/library" "$home_lower/desktop" \
</file context>
- Electron guard: both-directions case-insensitive process match (no fail-open on name mismatch) - DeviceSupport keep-N: version-ordered (sort -rV), not mtime; AI loop same - Symlinked retention roots refused (keep-N, AI, DiagnosticReports, Electron base) - Handoff age gate content-aware (skips if any descendant <60min) - NUL-delimited find enumeration in all find-driven loops - Unresolved Current -> skip framework suggestions - Survey: Electron vs browser-profile caches labeled separately - trash-items exit codes: previewed/missing counters, documented contract
|
All review findings addressed in
Upstream test suite grew 68 → 76 with regression tests for each of these (version-ordering-beats-mtime, both-direction process matching, symlinked-base refusal, content-fresh Handoff buffers, exit-code contract). Full changelog: https://github.com/JubaKitiashvili/mac-storage-cleaner/blob/main/CHANGELOG.md#201--2026-08-11 |
|
All six fixes are verified in the diff — the implementations match the descriptions exactly:
Clean release. LGTM. Tip: You can customize Greptile's behavior for this repo with |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Literal metachar-proof process probe (ps + grep -iF; regex pgrep -f dropped) - Control-char dir names fail closed in retention enumeration - Handoff freshness scan rc-aware (unreadable descendant => skip) - DiagnosticReports restricted to report artifacts (+Retired) - survey -mindepth 2 alignment - Subtree denies for /System /bin /sbin /dev /private/var/db
|
Round-2 findings addressed in Fixed (5):
Rebuttal (1): "stock macOS sort does not implement Suite is now 80 tests upstream, with regressions for each item above. Changelog: https://github.com/JubaKitiashvili/mac-storage-cleaner/blob/main/CHANGELOG.md#202--2026-08-11 |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… pipeline Retired/ is no longer rm -rf'd as a unit: its report files are enumerated individually through the same age/whitelist/validate/dry-run checks as top-level reports; the directory itself is never deleted.
|
Addressed in |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…y + partial-honesty branch
|
Addressed in |
| version_sorted_children () { | ||
| local dir="$1" d | ||
| [ -d "$dir" ] || return 0 | ||
| if ! printf '1\n' | sort -V >/dev/null 2>&1; then return 0; fi |
There was a problem hiding this comment.
On the documented default macOS environment, BSD sort rejects -V, so this branch emits no children and both DeviceSupport and AI CLI retention loops remove nothing. Older versions remain on disk and the cleaner provides no warning that the advertised retention cleanup was skipped.
…on never silently no-ops)
|
@greptile-apps re "retention silently becomes a no-op" — two parts: On the factual claim: stock macOS On the fair underlying concern: a silent keep-everything on any hypothetical |
|
Fair correction on both counts — you're right that BSD sort inherited The v2.0.5 fallback ( |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…onents, fail-closed on overflow
|
Valid catch on the fallback's component depth — addressed in
All three paths run under a |
|
The implementation in
The |
BREAKING: clean-safe.sh previews by default; deleting requires --apply. trash-items.sh refuses bulk batches (>100 items / 5GB) without --force and rejects unknown flags instead of treating them as paths. Also: portable skill-directory resolver (works when installed by any agent, not just Claude Code), portable frontmatter, declared-behavior section for skill scanners, 131 tests.
|
Updated to v3.0.0 ( The reason for the major version: the destructive default is gone.
The fix that makes it work outside Claude Code. Every command block in SKILL.md previously resolved the skill directory as For the security scan: SKILL.md now carries a mechanism-level declared behavior section enumerating every destructive operation (the exact Upstream: 131 tests, CI on macos-latest (bats + shellcheck), tag v3.0.0. Changelog: https://github.com/JubaKitiashvili/mac-storage-cleaner/blob/main/CHANGELOG.md#300--2026-08-14 — happy to adjust anything. |
| elif printf '%s\n' "$_pt" | LC_ALL=C grep -qiF "$app"; then | ||
| st=0 |
There was a problem hiding this comment.
When an idle app has a generic Application Support directory name such as Code, grep -qiF can match that text in an unrelated process's executable path or arguments, causing the cleaner to classify the app as running and skip its cache directories.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cli-tool/components/skills/productivity/mac-storage-cleaner/SKILL.md">
<violation number="1" location="cli-tool/components/skills/productivity/mac-storage-cleaner/SKILL.md:205">
P2: The doc says a preview with `MSC_DRY_RUN=1` is "never refused", but `trash-items.sh` does not skip the bulk guard in dry-run mode. The bulk check at line 117 (`if [ "$FORCE" != 1 ] && { [ "$eligible_n" -gt "$MAX_ITEMS" ] || ... }; then ... exit 4`) never tests `DRY`, so a batch over the 100-item/5 GB cap is refused (exit 4) even with `MSC_DRY_RUN=1`. An agent following this instruction to preview a large batch will be refused and pushed toward `--force` (which also disables the item cap). Either make the guard honor dry-run (refuse only real runs) or fix the doc to say that the 100-item/5 GB cap applies even to previews.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| **Bulk operations need confirmation.** `trash-items.sh` refuses a batch of more than 100 | ||
| eligible items or 5 GB and exits 4, because several agents run shell commands without | ||
| asking the user first. Show the user the list (a preview with `MSC_DRY_RUN=1` is never | ||
| refused), get their explicit go-ahead, then re-run with `--force` as the first argument. |
There was a problem hiding this comment.
P2: The doc says a preview with MSC_DRY_RUN=1 is "never refused", but trash-items.sh does not skip the bulk guard in dry-run mode. The bulk check at line 117 (if [ "$FORCE" != 1 ] && { [ "$eligible_n" -gt "$MAX_ITEMS" ] || ... }; then ... exit 4) never tests DRY, so a batch over the 100-item/5 GB cap is refused (exit 4) even with MSC_DRY_RUN=1. An agent following this instruction to preview a large batch will be refused and pushed toward --force (which also disables the item cap). Either make the guard honor dry-run (refuse only real runs) or fix the doc to say that the 100-item/5 GB cap applies even to previews.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli-tool/components/skills/productivity/mac-storage-cleaner/SKILL.md, line 205:
<comment>The doc says a preview with `MSC_DRY_RUN=1` is "never refused", but `trash-items.sh` does not skip the bulk guard in dry-run mode. The bulk check at line 117 (`if [ "$FORCE" != 1 ] && { [ "$eligible_n" -gt "$MAX_ITEMS" ] || ... }; then ... exit 4`) never tests `DRY`, so a batch over the 100-item/5 GB cap is refused (exit 4) even with `MSC_DRY_RUN=1`. An agent following this instruction to preview a large batch will be refused and pushed toward `--force` (which also disables the item cap). Either make the guard honor dry-run (refuse only real runs) or fix the doc to say that the 100-item/5 GB cap applies even to previews.</comment>
<file context>
@@ -131,6 +199,14 @@ the path is on the tool's deny list (system/user roots) — never work around a
+**Bulk operations need confirmation.** `trash-items.sh` refuses a batch of more than 100
+eligible items or 5 GB and exits 4, because several agents run shell commands without
+asking the user first. Show the user the list (a preview with `MSC_DRY_RUN=1` is never
+refused), get their explicit go-ahead, then re-run with `--force` as the first argument.
+Never pass `--force` pre-emptively. If `du` can't fully measure the batch (some paths are
+unreadable), the size guard is skipped for that run — with an on-screen warning and a
</file context>
|
Thanks — three findings came back on v3.0.0. One was a real fail-open and is fixed in 1. cubic P2 — The expansion errored, the enclosing 2. cubic P2 — a preview is refused by the bulk guard. Does not reproduce. The cap is inside the dry-run guard: 3. greptile P1 — the substring process guard can match an unrelated process. True, and deliberate. Upstream: v3.0.1 tagged, 134 tests, CI green on macos-latest. |
1 participant
Update of the merged skill (#721) to v2.0.0 — a substantial safety-hardening release. Upstream repo/changelog: https://github.com/JubaKitiashvili/mac-storage-cleaner/blob/main/CHANGELOG.md
What changed (all 7 files, +1012/−66):
~/.ssh/~/.aws/~/.gnupg— the validator refuses them even if the reasoning layer were ever wrong, and it now also runs as defense-in-depth inside every automatic deletion loop.--dry-run(full preview, zero deletions/log writes, guards applied identically to a real run), a user whitelist file (case-insensitive, protects subtrees), and fail-closed process guards (Xcode toolchain, Gradle daemon, running Electron apps — "can't tell" always means "skip")./usr/bin/trash→ Finder → same-volumemv) with per-method audit logging; deletion refuses to run if the audit log is unwritable.size?, whitelist-aware survey totals, 5 MB log rotation.For the security scan (same notes as #721, still true): the skill makes no network calls and no credential access; every
rm -rfis gated behind a hard-coded allowlist plus the new deny-list validator;~/.ssh/keychain/backup paths appear only inside the refuse-to-touch protection lists. It now ships with a 68-test bats suite upstream, including a property-tested dangerous-path corpus (every corpus entry must be refused) and adversarial symlink cases — audited by an independent multi-model review panel before release.Happy to adjust anything — thanks for maintaining this!
Summary by cubic
Upgrades
mac-storage-cleanerto v3.0.1 to prevent unintended deletions and enforce bulk protections. Old:clean-safe.shdeleted by default; New: it previews by default and requires--apply. Also fixes an octal parsing bug in the Trash size cap so bulk limits are always enforced.--applyfor real deletions; unknown flags or multiple args now exit 2.trash-items.shrefuses >100 items or >5GB without--force; cap enforcement fix prevents fail-open cases.cli-tool/components/skills/productivity/mac-storage-cleaner); addsagents/openai.yaml; no new components; nodocs/components.jsonregeneration.MSC_WHITELIST_FILE,MSC_TRASH_BIN,MSC_DEVICE_SUPPORT_KEEP,MSC_AI_AGENTS_KEEP,MSC_DRY_RUN,MSC_ALLOW_UNLOGGED; no new secrets.Written for commit 627e088. Summary will update on new commits.