You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
preserve custom files and keep the CLI available when fail-safe cleanup needs a retry
Safety hardening
refuse broad, symlinked, and non-directory purge targets
compare existing purge targets to roots, HOME, HOME ancestors, and shared temp by device/inode identity, including case aliases, symlinks, hardlinks, and bind mounts
retain conservative lexical checks for nonexistent paths and fail closed when an existing safety relationship cannot be inspected
treat every malformed host config as fail-safe because managed spellings cannot be classified reliably; --skip-hosts is the only explicit bypass
provide --skip-hosts to leave plugin managers and shared/recognizable host config for manual cleanup when a host CLI or config is broken
capture the initial data-directory device/inode, then re-run identity, root, HOME, ancestor, temp, and symlink guards immediately after awaited host commands and before the synchronous data-removal block
refuse a swapped data directory without touching either the original directory or its replacement target, and retain the CLI for retry
edit shared OpenCode JSON/JSONC without dropping unrelated entries or comments; preserve indentation, EOL, and trailing-newline style in strict JSON rewrites
preserve unknown skill layouts, vendor content, scoped package caches, and external save paths
detect disabled Claude and Droid plugin installations and installer-adopted relocated Codex hooks
restore the exact original Windows user PATH if self-delete scheduling fails
schedule Windows parent removal only for the dedicated %LOCALAPPDATA%\plannotator install directory, never a shared executable parent
unlink installer-owned files, symlinks, and hardlink entries directly so recursive removal can only run on real directories and never traverse a link target
report partial failures instead of claiming a complete uninstall
Validation
focused uninstall/CLI tests: 54 passed, 0 failed (including case-variant HOME and post-host-command path-swap regressions)
full bun test suite: 2,664 passed, 198 skipped, 0 failed
bun run typecheck
marketing production build
Windows x64 and ARM64 compiled CLI cross-builds
workflow YAML parsing and git diff --check
self-review completed after the adversarial review fixes
Windows QA
focused uninstall tests run on windows-latest for every PR
the existing compiled-binary Windows smoke runs preserve-data and purge flows
coverage verifies PATH cleanup/rollback, delayed running-EXE deletion, target-preserving symlink/hardlink cleanup, and that legacy ~/.local/bin installs never schedule shared-parent removal
all filesystem fixtures use isolated temporary homes; the compiled smoke restores the disposable runner user PATH in finally
Full suite 2655 pass / 0 fail, typecheck clean. Real e2e runs in isolated fake $HOMEs, including a compiled binary to exercise POSIX self-delete. Verdict: needs changes. One blocking defect, otherwise this held up under maximum suspicion.
Blocking
1. Purge containment guards are bypassable on macOS (case-insensitive FS). samePath (packages/server/uninstall.ts:1593) and isAncestorOrSame (:1650) case-fold only when platform === "win32". macOS APFS is case-insensitive by default, so a case-variant PLANNOTATOR_DATA_DIR resolves to the same directory but compares unequal, and every guard in getDataDirSafetyIssue silently passes.
Reproduced, not theoretical. With PLANNOTATOR_DATA_DIR set to an uppercased $HOME, purge deleted ~/plans, ~/history, ~/drafts, ~/sessions, and ~/config.json out of the sandbox home. The exact-case equivalent correctly refuses. The README advertises "purge refuses the home directory" as a safety property; the last-line-of-defense guard cannot fail on the project's primary platform.
Suggested fix: compare device+inode identity (statSync(a).dev/ino === statSync(b).dev/ino) instead of string paths. That is immune to case, symlinks, hardlinks, and bind mounts. At minimum extend the case-fold to darwin. Add a regression test with a case-variant data dir.
Should fix (can be follow-ups, but 2 and 4 are cheap now)
2. Unrelated malformed host config permanently dead-ends uninstall. Any non-strict-JSON host file (e.g. a ~/.gemini/settings.json with a // comment and zero Plannotator content) pushes an error, and binary removal is gated on zero errors, so plannotator uninstall exits 1 forever with no escape hatch. Same dead-end when a detected host CLI binary is missing. Suggest escalating to error only when the file actually contains a managed entry, plus a --skip-hosts-style flag.
3. writeJson reformats the whole user settings.json via JSON.stringify(v, null, 2). Keys and values survive, but user formatting does not. The OpenCode path already does surgical jsonc applyEdits; settings.json deserves the same, or at least indent detection.
4. Windows self-delete removes the binary's parent dir unconditionally. If the exe lives in %USERPROFILE%\.local\bin, that shared directory gets a Remove-Item -Force (mitigated: non-recursive, so it only succeeds when empty). Restrict to windowsInstallDir.
5. Test gaps on the dangerous paths: no symlink-inside-removal-root test (behavior verified correct manually: links to ~/Documents and / were unlinked, targets intact), no case-variant containment test, dry-run purity asserted with one existsSync rather than a tree snapshot, and the POSIX self-delete of a compiled binary is never smoke-tested in CI (Windows-only gate).
6. Hook recognition is narrower than hook installation.isManagedHook matches only bare plannotator or the 4 hard-coded paths, while install.sh's Codex merge adopts any absolute path basenamed plannotator. A relocated binary leaves a hook pointing at a deleted file.
Verified clean
Dry-run purity: byte-exact tree + content snapshot before/after, identical. Dry-run enumeration matches the real run item for item on the same fixture.
CI: the uninstall smoke gates releases via smoke-binaries; no existing gate weakened.
Recommendation: fix 1 with an identity-based comparison and a regression test, ideally take 2 and 4 in the same pass, then this is merge-ready. Items 3, 5, 6 can land as follow-ups.
Verdict: merge as-is. Same reviewer as the base review, re-running the original attacks against a freshly compiled binary. Full suite 2664 pass / 0 fail; focused uninstall/cli/install tests 146 pass. No workflow or install-script bytes changed in this delta, so the #1157/#1162 invariants and CI gates are untouched by construction.
The blocker is genuinely fixed and fails closed. The exact case-variant repro (uppercased PLANNOTATOR_DATA_DIR against a sandbox home) now refuses before any removal, with all previously destroyed directories intact. The new device/inode mechanism was attacked directly: a missing data dir proceeds as a clean no-op, an unreadable path refuses with an identity-could-not-be-verified error (fails closed), and a legitimately mounted separate volume is not false-refused since identity requires both dev and ino to match. The ancestor walk compares inodes, which is what catches /USERS as an alias of /Users. The full seven-way refusal matrix passes end to end.
The new TOCTOU revalidation is real and correctly scoped. Identity is captured before any await, and after host cleanup the entire safety chain re-runs plus a dev/ino comparison against the initial capture, gating both purge and default-mode sidecar removal. Verified with a hostile fake host CLI that swaps the data dir for a symlink during the awaited plugin uninstall: refused, original and replacement data intact, binary preserved. The removal block itself is await-free, so no same-process yield reopens the window; the residual (an external process winning a microsecond race on a prefix directory) is not closable in Node and is documented in-code.
Both dead-ends are escapable. The malformed-config and missing-host-CLI repros still error safely but now name --skip-hosts in the message; with the flag, uninstall completes and leaves the malformed file byte-identical. Files that do contain managed entries still error rather than being skipped, pinned by an adversarial test using an escaped "plannotator" spelling a naive substring scan would miss. The flag is scoped to host config only and documented in help and docs.
Windows self-delete is restricted to the managed install dir, with the parent nulled for a legacy ~/.local/bin binary and a sibling-survival test. The fragile detach and for-loop syntax fixes from the earlier commits are byte-identical.
The link-unlink fix is a Windows-semantics correction (directory reparse points routed through unlinkSync instead of non-recursive rm). The original symlink battery still passes on the compiled binary (links to /, nested links, targets intact), now extended with hardlinks: removing one name leaves the other name's content untouched through both removal paths.
Every new regression test fails on the old code (verified by running the new test file against the old implementation), including the case-variant test, which self-skips on case-sensitive filesystems and runs on the dedicated Windows job and macOS. Dry-run byte-purity is now pinned in-repo by a content-snapshot assertion. Two of the base review's should-fixes (settings.json indentation preservation, relocated-hook recognition) were also fixed unprompted and verified.
One should-fix to record as a follow-up, not a blocker: --skip-hosts leaves a dangling hook that breaks the host agent. It removes the binary while leaving ~/.claude/settings.json hooks pointing at it, so Claude Code invokes a nonexistent command on every ExitPlanMode. The warning is honest but generic; running detection-only under the flag and listing the specific preserved files would fix it. Three nits: relocated-Codex matching case-folds on Linux and can adopt a same-named third-party hook (consistent with install.sh's own rule), --skip-hosts help-text column misalignment, and a README line-break artifact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
plannotator uninstallcommand--purge,--yes,--dry-run, and recovery-oriented--skip-hostsmodesSafety hardening
--skip-hostsis the only explicit bypass--skip-hoststo leave plugin managers and shared/recognizable host config for manual cleanup when a host CLI or config is broken%LOCALAPPDATA%\plannotatorinstall directory, never a shared executable parentValidation
bun testsuite: 2,664 passed, 198 skipped, 0 failedbun run typecheckgit diff --checkWindows QA
windows-latestfor every PR~/.local/bininstalls never schedule shared-parent removalfinally