Repository navigation
fix(implode): preserve unrelated data when removing installations - #3228
jong-kyung wants to merge 5 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
|
Could you update the PR description to explain the ownership and deletion rules, including how existing installations are handled? With the current implementation, users upgrading in place from 1.4.0 will have non-empty directories without .vp-owned. Setup leaves those directories unmarked, so implode refuses to remove them, even with --yes. This affects normal upgrades, not just custom directory overrides. Please clarify the intended behavior for fresh installs, upgrades from older versions, and custom directories. In particular, how should existing users safely uninstall after upgrading? We should consider the compatibility impact explicitly and cover the upgrade-to-uninstall flow with regression tests, while preserving protection against deleting unrelated data. |
|
Thanks for testing this and for the detailed feedback 🫡 I'm reworking the removal approach so that existing users can still uninstall after upgrading. I'll request another review once I've verified it more thoroughly. |
Identify legacy CLI version directories using matching package metadata and frozen installation receipts instead of claiming their parent roots. Preserve unverified data and report the paths left for manual inspection. Keep protected paths and unowned nested categories safe, and remove only verified shims from shared bin directories. Cover the legacy upgrade refresh handoff, partial cleanup, invalid receipts, and symlink boundaries.
jong-kyung
changed the title
|
@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". |
vp implodecould delete unrelated files when a directory override pointed to the user home or another non-Vite+ directory. This change limits removal to verified Vite+ directories and shims, while allowing existing users to uninstall after upgrading.For example, setting
VP_HOMEto$HOME/Documentsmust not authorize deletion of personal documents. Requiring.vp-ownedeverywhere avoids that deletion, but also blocks users upgrading from older versions whose installation directories have no ownership markers.Approach
Check ownership before changing files, shell profiles, or PATH. An upgrade does not grant Vite+ ownership of preexisting directory contents.
--yes.Installation and removal rules
vp implodebehavior.vp-ownedmarkers.Ownership covers a whole directory, so recursive removal also deletes files added inside that verified unit. It does not extend to an unmarked parent or override the nested-category checks.
Uninstall after upgrading
Existing users can run
vp implodeafter upgrading without manually adding ownership markers. For an unmarked installation:currentlink, and owned shims. Clean the matching shell entries.This allows a successful partial uninstall without treating an old installation's parent directory as disposable.
--yesskips confirmation, not ownership checks.