Fix three safety bugs: trash retention, silent flag typos, blunt chown -R#1
Conversation
'vole clean' advertises a 7-day grace period before permanently deleting trashed items, but retention was computed from each item's original modification time. Trashing a 2-year-old folder and running clean an hour later would permanently rm -rf it, despite the grace period. move_to_trash() now records a deletion timestamp when items are trashed: a sidecar file under ~/.Trash/.vole-trash-meta on macOS, and by relying on the DeletionDate gio already writes to the freedesktop .trashinfo sidecar on Linux (which was being ignored in favor of mtime). clean.sh's trash sweep now expires items based on that recorded deletion time. Items with no known deletion time (trashed before this existed, or dropped in by another tool) are never deleted on the run that first notices them — a timestamp is backfilled instead, starting the grace period from that point forward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… them clean.sh, optimize.sh, and purge.sh silently swallowed unknown flags in their argument-parsing loops. A mistyped --dryrun (instead of --dry-run) was ignored rather than rejected, so the command ran a REAL destructive pass with no warning. purge.sh was worst: it had an explicit `-*) ;;` no-op case that discarded any unrecognized flag on purpose. update.sh already guarded against this (an explicit `-*) echo "Unknown option..."; exit 1 ;;` case). Apply the same pattern to the three destructive scripts so an unrecognized flag is a hard error (non-zero exit) instead of a silent fallthrough. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
opt_linux_home_ownership() (vole optimize's "Home Directory Ownership" task) ran `sudo chown -R "$(id -u):$(id -g)" "$HOME"` whenever $HOME's top-level owner didn't match the current user. That recursive chown also rewrites ownership inside rootless container storage under $HOME (e.g. ~/.local/share/containers for podman, ~/.local/share/docker for rootless Docker), which is intentionally subuid/subgid-mapped — corrupting it. The repair now walks $HOME's top-level entries, chowns (recursively) only the ones whose owner actually doesn't match, and skips known container storage directories entirely. $HOME itself is fixed non-recursively. No entry is touched unless it's actually misowned, and container storage is never touched at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ada09b9d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [[ "$skip" == "true" ]] && continue | ||
| entry_owner=$(get_file_owner "$entry") | ||
| [[ -z "$entry_owner" || "$entry_owner" == "$USER" ]] && continue | ||
| if sudo chown -R "$(id -u):$(id -g)" "$entry" 2> /dev/null; then |
There was a problem hiding this comment.
When the misowned top-level entry is a parent of the excluded storage, e.g. $HOME/.local, the exact-match skip above does not fire because find only yields $HOME/.local at maxdepth 1. This recursive chown therefore still descends into $HOME/.local/share/containers or Docker storage and rewrites rootless container subuid/subgid mappings, which is the corruption this change is trying to avoid; prune those paths from the recursive chown or use find with exclusions.
Useful? React with 👍 / 👎.
| deletion_date=$(sed -n 's/^DeletionDate=//p' "$trashinfo" 2> /dev/null | head -1) | ||
| [[ -n "$deletion_date" ]] || return 0 | ||
| epoch=$(date -d "$deletion_date" +%s 2> /dev/null || echo "") | ||
| [[ "$epoch" =~ ^[0-9]+$ ]] && echo "$epoch" |
There was a problem hiding this comment.
When a Linux .trashinfo file has a DeletionDate that date -d cannot parse, this final [[ ... ]] returns 1. Because bin/clean.sh runs with set -e and assigns deleted_epoch=$(trash_deleted_epoch "$item"), one malformed trash sidecar aborts the whole clean instead of taking the unknown-time backfill path; return success with no output for invalid timestamps, the same way missing timestamps are handled.
Useful? React with 👍 / 👎.
| local name | ||
| name="$(basename "$trashed_path")" | ||
| if is_macos; then | ||
| local meta_file="$HOME/.Trash/$VOLE_TRASH_META_DIR_NAME/$name" |
There was a problem hiding this comment.
On macOS, this lookup reuses any sidecar with the same basename, even if the original trash item was restored or deleted outside Vole and the sidecar remained. If a new item with that basename is later put in Trash by Finder or another tool, clean can apply the stale old epoch and permanently delete the new item on its first run, bypassing the intended grace period; include a unique file identity or clear orphaned sidecars before trusting them.
Useful? React with 👍 / 👎.
| else | ||
| failed=$((failed + 1)) | ||
| fi | ||
| done < <(find "$HOME" -mindepth 1 -maxdepth 1 -print0 2> /dev/null) |
There was a problem hiding this comment.
When the home directory itself is misowned and not searchable by the user, this unprivileged find returns no entries while its error is discarded. The script then only chowns $HOME non-recursively and reports that ownership is already optimal, leaving the root-owned contents broken until a second optimize run; chown $HOME before walking it or enumerate with sudo.
Useful? React with 👍 / 👎.
| { | ||
| echo "[Trash Info]" | ||
| echo "Path=$trashed_path" | ||
| echo "DeletionDate=$iso" | ||
| } > "$info_dir/$name.trashinfo" 2> /dev/null || true |
There was a problem hiding this comment.
When a Linux .trashinfo sidecar exists but lacks DeletionDate, trash_deleted_epoch treats it as unknown and this block rewrites the whole sidecar with Path set to the internal Trash/files location. Desktop trash tools use Path as the original restore target, so a clean run can destroy the only restore location metadata for that item; preserve the existing Path and only add the backfilled timestamp, or keep Vole metadata separately.
Useful? React with 👍 / 👎.
|
@cursor can you review these findings and if all are useful implement codex's findings. |
|
Reviewed Codex’s findings; all five were useful and are implemented. Changes made:
Verification:
Committed and pushed the updates to the current branch.   |
Co-authored-by: Fischer <fschrhunt@users.noreply.github.com>
Co-authored-by: Fischer <fschrhunt@users.noreply.github.com>
Co-authored-by: Fischer <fschrhunt@users.noreply.github.com>
2 participants
Summary
Three safety bugs found in a security review, each fixed as a separate commit:
Trash retention keyed off file mtime, not deletion date (data loss).
vole cleanadvertises a 7-day grace period before permanently deleting trashed items, but retention was computed from each item's original mtime. Trashing a 2-year-old folder and runningvole cleanan hour later would permanentlyrm -rfit.move_to_trash()now records a deletion timestamp when items are trashed (a sidecar file under~/.Trash/.vole-trash-metaon macOS; theDeletionDategio already writes into the freedesktop.trashinfosidecar on Linux, which was previously being ignored).clean.sh's trash sweep now expires items based on that recorded deletion time. Items with no known deletion time (pre-existing trash, or dropped in by another tool) are never deleted on the run that first notices them — a timestamp is backfilled instead, starting the grace period from that point forward.Mistyped flags silently fell through in destructive commands (data loss).
clean.sh,optimize.sh, andpurge.shignored unrecognized flags in their arg-parsing loops — a mistyped--dryrun(instead of--dry-run) ran a REAL destructive pass with no warning.purge.shwas the worst offender, with an explicit-*) ;;no-op that discarded any unknown flag on purpose.update.shalready guarded against this with an explicit-*) echo "Unknown option..."; exit 1 ;;case; the same pattern is now applied to the three destructive scripts, so an unrecognized flag is a hard error instead of a silent fallthrough.Blunt
sudo chown -R "$HOME"could flatten container storage.vole optimize's "Home Directory Ownership" task ran a single top-level owner check that, if it failed, triggeredsudo chown -R "$(id -u):$(id -g)" "$HOME"— rewriting ownership everywhere under$HOME, including rootless container storage (e.g.~/.local/share/containersfor podman,~/.local/share/dockerfor rootless Docker) that is intentionally subuid/subgid-mapped. The repair now walks$HOME's top-level entries,chowns (recursively) only entries whose owner actually doesn't match, and skips known container storage directories entirely.$HOMEitself is fixed non-recursively.Verification
shellcheck bin/*.sh lib/core/common.sh lib/optimize/tasks.sh— no new findings introduced (remaining output is pre-existing SC1091/SC2088/SC2034 noise already present onmain).bash -non all modified/related scripts — syntax clean.clean.sh --dryrun,optimize.sh --dryrun,purge.sh --dryrun(misspelled) against a throwaway$HOME. All three now printUnknown option: --dryrunand exit 1 instead of running a real destructive pass. Confirmed correctly-spelled flags (--dry-run,--list,--help,--min=) still work and exit 0.$HOME, created a file with a 2.5-year-old mtime, trashed it viamove_to_trash, and confirmed the recorded deletion timestamp was "now" (not the old mtime). Ranclean.shwith a 7-day retention window — the item correctly survived (mtime is irrelevant now). Separately, backdated a trash item's deletion timestamp to 10 days ago (with a fresh mtime) and confirmedclean.shcorrectly deleted it — proving retention now keys off deletion time in both directions. Also verified a legacy trash item with no deletion-time record survives its firstclean.shrun (conservative backward-compat) and gets a timestamp backfilled. Additionally verified the Linux.trashinfoDeletionDateISO-8601 parsing logic in isolation with GNUdate -d(installed viabrew install coreutilssince testing ran on macOS).Not merging — opening for review per instructions.