Repository navigation
fix(implode): prevent deletion of protected and unowned directories - #3228
jong-kyung wants to merge 4 commits into
Conversation
Reject protected deletion targets before modifying shell profiles, PATH, or managed files, including with --yes. Check canonical paths to catch symlink aliases and validate all roots before starting split-layout cleanup. Add regression coverage for protected roots, normal installations, symlink aliases, and preservation of files when validation fails.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
fengmk2
left a comment
fengmk2
left a comment
There was a problem hiding this comment.
The home/root protection works in the targeted Linux tests, but I would like us to complete the directory ownership protection in this PR before merging. A mistaken VP_HOME override can still delete unrelated data. Please address the inline comment and add regression coverage for unowned directories.
Require ownership markers for every existing data, cache, config, and state root before changing files, shell profiles, or PATH. Validate nested roots independently and collapse them only when building the final recursive deletion targets. Preserve the home and filesystem root safeguards. Check that new installation roots are empty before recording ownership. Keep nonempty legacy roots unmarked so existing installations remain usable without granting permission to delete unrelated contents. Cover ownership checks, nested and split roots, fresh setup, and legacy setup with isolated unit tests and CLI snapshots.
|
@codex review |
Recognize an existing current entry without following its target, matching installation directory resolution even when the active binary is missing. Keep repaired legacy roots unmarked so implode still refuses their removal. Extend the existing installer CI cases to verify ownership is not claimed when reusing a legacy root or repairing a dangling junction.
Keep ownership checks focused on recursive removal. Setup may use an existing nonempty root without marking it as owned, so implode still refuses to delete it. Remove the unnecessary existing-install distinction. Verify that setup succeeds without claiming personal files and that implode preserves those files and shell configuration. Keep preflight coverage for invalid roots and update the doctor cache-directory snapshot.
|
I've made some changes to address your feedback, but I'm not sure if this is the right approach. I'd appreciate it if you could take another look 🙇♂️ |
jong-kyung
changed the title
Summary
vp implodecould recursively delete the user home or unrelated directories selected through directory overrides.Reject the user home, its ancestors, and filesystem roots, including symlink aliases. Require ownership verification for every existing data, cache, config, and state root, including nested roots, before modifying files, shell profiles, or PATH. These checks also apply with
--yes.Setup creates
.vp-ownedmarkers only for new or empty roots. Installation into nonempty unmarked roots remains supported, butimploderefuses to delete them. This also applies to older installations without ownership markers, which must be inspected and removed manually.