[codex] Add generic three-stage residual cleanup - #2
Conversation
clawdbot502
left a comment
There was a problem hiding this comment.
Blocking safety issue: configured XDG roots can promote protected or overly broad locations into approved residual-deletion roots.
Reproduction against this PR on macOS:
XDG_CONFIG_HOME=/Applications python - <<'PY'
from pathlib import Path
from cleanapp.residual_scanner import is_approved_residual_path
print(is_approved_residual_path(Path("/Applications/Clash Verge.app")))
PYThis returns True. I also constructed a ResidualIdentity with the matching display name and an independent bundle signal; ResidualScanner(system_roots=()).scan(identity) placed that application in confirmed with display name Clash Verge evidence. The same root-policy gap applies to values such as /Users, /Library, /tmp, or another arbitrary high-value directory because _root_specs() only rejects roots under /System and /usr. validate_deletion() protects the broad root itself but generally permits its direct children, so the second residual confirmation can reach recursive deletion for a child of a misconfigured XDG root.
Please harden the root policy before merge:
- Reject configured XDG roots that resolve to protected/broad roots (reuse or align with
safety.PROTECTED, including/,/Applications,/Library,/Users,/opt,/private,/tmp,/var, and the home container itself). - Ensure
is_approved_residual_path()independently enforces the same policy at deletion time. - Consider making non-home XDG roots review-only unless they satisfy a narrowly defined ownership/scope policy.
- Add regression tests for hostile/misconfigured XDG values such as
/Applications,/Users, and/Library, covering both scan classification and the deletion boundary.
The five CI jobs are green and the overall three-stage design is thoughtful, but a cleanup tool must fail closed when environment-controlled roots widen deletion scope. I cannot approve or merge this revision until this boundary is fixed.
Superseded by repository-owner direction: broad and non-home XDG roots are an intentional cleanup capability. See the follow-up review comment for the revised safety requirements.
clawdbot502
left a comment
There was a problem hiding this comment.
Repository-owner clarification — this supersedes the previous change request.
The following two requirements are withdrawn:
- Do not reject configured XDG roots solely because they resolve to broad locations such as
/Applications,/Library,/Users,/opt,/private,/tmp,/var, or outside the current home. Thorough cleanup of explicitly configured local locations is an intended product capability. - Do not automatically downgrade non-home XDG-root matches to review-only solely because they are outside the home directory. They may remain confirmed when the existing strong identity rules classify them as confirmed.
The earlier review has been dismissed. The intended safety contract is now capability-preserving:
- Never delete the configured root itself; only direct children may become candidates.
- Keep system-critical protections for
/System,/usr, and the existing protected-path deletion boundary. - Weak, short-name, shared-container, and platform-managed matches must remain review-only.
- Show every confirmed path and require the separate residual
y/Nconfirmation before deletion. - At deletion time, independently revalidate the approved-root relationship plus device, inode, and file type captured during preview.
- Never invoke
sudoor silently elevate privileges. - Preserve inaccessible-root and failure reporting so the final result does not claim a complete cleanup when locations could not be scanned.
Tests for custom roots such as /Applications, /Library, and /Users should document this intentional behavior and verify the invariants above, rather than require blanket rejection. No change is requested merely for allowing broad or non-home XDG roots.
clawdbot502
left a comment
There was a problem hiding this comment.
Approved under the repository-owner cleanup policy clarified in review 4692216120. Broad and non-home XDG roots are intentional capabilities; the implementation retains direct-child scoping, explicit residual confirmation, deletion-time device/inode/type revalidation, no automatic sudo, and truthful incomplete-scan reporting. All CI checks pass.
Summary
y/Nconfirmation for confirmed residuals and always run a fresh verification scan #3Safety boundaries
~/.localand~/Library/Preferences/ByHostsudoautomatically; permission limits remain visible and non-fatalVerification
python -m compileall -q cleanapp testsuv pip checkuv buildproduced sdist and wheeluvx --from . cleanapp --version,rules, anddoctor253661133+BruceL017@users.noreply.github.comfor author and committerThe implementation deliberately adds no Poe-specific rule; Poe's demonstrated residuals are covered by the generic identity-linked scanner while Chrome website data remains outside the scan boundary.