Add friendly uninstall flow#33
Conversation
*Total -- 2,498.99kb -> 2,348.75kb (6.01%) /public/podcli-logo-transparent.png -- 12.66kb -> 5.68kb (55.13%) /src/ui/public/podcli-logo-transparent.png -- 12.66kb -> 5.68kb (55.13%) /public/podcli-icon.png -- 5.50kb -> 2.48kb (54.83%) /src/ui/public/podcli-icon.png -- 5.50kb -> 2.48kb (54.83%) /public/podcli-logo-full.png -- 14.01kb -> 6.64kb (52.61%) /public/podcli-icon-transparent.png -- 3.42kb -> 1.66kb (51.54%) /src/ui/public/podcli-icon-transparent.png -- 3.42kb -> 1.66kb (51.54%) /public/promo.gif -- 2,441.83kb -> 2,322.47kb (4.89%) Signed-off-by: ImgBotApp <ImgBotHelp@gmail.com>
…-option Add friendly uninstall flow
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds uninstall support across the CLI, installers, tests, and documentation, plus a shared executable-suffix helper used by provisioning code. ChangesUninstall feature
Executable suffix helper
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant podcliCLI
participant Filesystem
User->>podcliCLI: podcli uninstall [flags]
podcliCLI->>podcliCLI: uninstallTargets()
podcliCLI->>podcliCLI: podcliLinks()
podcliCLI->>User: print targets and prompts
podcliCLI->>Filesystem: unlink symlinks / remove directories
podcliCLI->>User: print result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cli/main.go (1)
396-408: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
os.UserHomeDir()instead ofos.Getenv("HOME")for cross-platform correctness.When
HOMEis unset (common on Windows, and possible in minimal environments),filepath.Join(os.Getenv("HOME"), ".local", "bin")degrades to a relative path (.local/bin), which won't resolve to the intended user bin directory.os.UserHomeDir()handles Windows (USERPROFILE) and other platforms consistently.♻️ Proposed fix
- for _, d := range []string{"/usr/local/bin", filepath.Join(os.Getenv("HOME"), ".local", "bin")} { + homeDir, _ := os.UserHomeDir() + for _, d := range []string{"/usr/local/bin", filepath.Join(homeDir, ".local", "bin")} { if d == "/usr/local/bin" && runtime.GOOS == "windows" { continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/main.go` around lines 396 - 408, The podcliLinks helper currently builds the user bin directory from os.Getenv("HOME"), which can be empty and produce an invalid relative path. Update podcliLinks to resolve the home directory with os.UserHomeDir() before joining ".local" and "bin", and handle any lookup error cleanly so the link scan still works cross-platform.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/main.go`:
- Around line 372-381: The running-binary guard in main’s target-removal loop is
comparing a directory target against the executable file path, so sameFile never
matches and the skip message is never reached. Update the check in the targets
loop to detect whether os.Executable() is located inside the directory being
removed (rather than exact file equality), then keep the existing
warning/continue behavior for that case so removing paths.BinDir() or other
parent directories safely avoids the active binary.
In `@install.ps1`:
- Around line 12-27: The uninstall path in install.ps1 can abort mid-cleanup
because Remove-Item runs under a global Stop error preference with no per-target
handling. Update the uninstall block to wrap each removal in the loop around
$binDir, runtime, models, and tools in try/catch so a locked file or missing
handle logs a warning and continues instead of terminating. Keep the PATH
removal behavior in the uninstall branch, but make the cleanup resilient in the
install.ps1 uninstall flow.
In `@install.sh`:
- Around line 29-43: The uninstall branch in install.sh currently relies on rm
commands under set -eu, so a cleanup failure can terminate before the final
status messages run. Update the --uninstall flow to explicitly check the results
of removing each symlink and the bin/runtime/models/tools directories, and emit
warnings or status output from that same block when a removal fails. Keep the
changes scoped to the uninstall path and use the existing identifiers like the
--uninstall branch, link removal loop, and bin_dir/home_dir cleanup variables so
the partial cleanup outcome is still reported.
---
Nitpick comments:
In `@cli/main.go`:
- Around line 396-408: The podcliLinks helper currently builds the user bin
directory from os.Getenv("HOME"), which can be empty and produce an invalid
relative path. Update podcliLinks to resolve the home directory with
os.UserHomeDir() before joining ".local" and "bin", and handle any lookup error
cleanly so the link scan still works cross-platform.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eb75cd5-6931-4705-9618-a7703c32c30b
📒 Files selected for processing (6)
README.mdcli/main.gocli/uninstall_test.goinstall.ps1install.shnpm/README.md
[ImgBot] Optimize images
There was a problem hiding this comment.
Nice, safe uninstall flow — data-preserving default, --dry-run/--yes/--purge, and the "only unlink when the symlink resolves to the managed binary" check are all the right instincts. One correctness issue worth fixing before merge, plus two optional cleanups.
1. (blocking) The running-binary guard is dead code and breaks Windows uninstall. See the inline comment on cli/main.go — sameFile(p, self) compares a directory against the executable, so it's never true; on Windows the running .exe can't be deleted and the user gets a raw warning instead of the intended "delete after exit" guidance. Inline comment has a concrete patch.
2. (optional) install.sh --uninstall is unreachable on Intel Macs. The --uninstall block sits after the darwin-amd64 hard-err (line 24), so a user who installed with an older script can't uninstall via curl … | sh -s -- --uninstall. Move the uninstall branch above the arch/target gating (it only needs home_dir/bin_dir).
3. (optional) Duplicate exeSuffix(). cli/main.go adds an exeSuffix() identical to the one in cli/internal/provision/provision.go. Harmless (different packages), but a shared paths.ExeSuffix() would avoid the copy.
# Conflicts: # cli/main.go # cli/uninstall_test.go # install.ps1 # install.sh
…-option-ff1c8r Add `podcli uninstall` command and installer uninstall support
|
Merged What changed:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
install.ps1 (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-ASCII ellipsis may render incorrectly without a BOM.
Static analysis flags a missing BOM for this non-ASCII-encoded file, driven by the
…character. Windows PowerShell 5.1 without a UTF‑8 BOM can misread this file under the legacy console codepage, corrupting output.🛠️ Proposed fix
-Write-Host "Installing podcli v$version ($target)…" +Write-Host "Installing podcli v$version ($target)..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install.ps1` at line 46, The PowerShell installer script’s Write-Host message uses a non-ASCII ellipsis, and the file is missing a UTF-8 BOM, which can cause Windows PowerShell 5.1 to misread it. Update install.ps1 so it is saved with a UTF-8 BOM, or remove the non-ASCII character in the Write-Host output and keep the script encoding consistent with PowerShell 5.1 expectations.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/main.go`:
- Around line 339-341: The unlink logic in podcliLinks should not remove the
running executable unless it is actually inside the managed bin directory.
Update the fallback that adds self to the targets so it only applies when
pathContains(paths.BinDir(), self) is true, or restrict the match to managed in
the uninstall flow, and make the same change in the other uninstall path that
uses podcliLinks so user-managed podcli symlinks outside paths.BinDir() are
preserved.
- Around line 371-383: The removal loop in main should not treat every
os.RemoveAll failure for a target containing self as only “running binary still
in use”; keep the runningInUse note, but also print or propagate the actual
removal error. Update the error handling around os.RemoveAll in main so failures
for BinDir() or home purge are surfaced even when pathContains(p, self) is true,
while still preserving the existing self-in-use message and final uninstall
guidance.
---
Nitpick comments:
In `@install.ps1`:
- Line 46: The PowerShell installer script’s Write-Host message uses a non-ASCII
ellipsis, and the file is missing a UTF-8 BOM, which can cause Windows
PowerShell 5.1 to misread it. Update install.ps1 so it is saved with a UTF-8
BOM, or remove the non-ASCII character in the Write-Host output and keep the
script encoding consistent with PowerShell 5.1 expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26632896-cf48-4c3e-964c-e93ce158ac63
⛔ Files ignored due to path filters (8)
public/podcli-icon-transparent.pngis excluded by!**/*.pngpublic/podcli-icon.pngis excluded by!**/*.pngpublic/podcli-logo-full.pngis excluded by!**/*.pngpublic/podcli-logo-transparent.pngis excluded by!**/*.pngpublic/promo.gifis excluded by!**/*.gifsrc/ui/public/podcli-icon-transparent.pngis excluded by!**/*.pngsrc/ui/public/podcli-icon.pngis excluded by!**/*.pngsrc/ui/public/podcli-logo-transparent.pngis excluded by!**/*.png
📒 Files selected for processing (6)
cli/internal/paths/paths.gocli/internal/provision/provision.gocli/main.gocli/uninstall_test.goinstall.ps1install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- install.sh
There was a problem hiding this comment.
The three original issues are fixed correctly (running-binary guard, install.ps1 try/catch, install.sh per-target rm). I checked CodeRabbit's two remaining open findings against the current code — both are real, here's my read plus the minimal patch:
A. podcliLinks can delete a symlink podcli didn't create — worth fixing
links matches managed or self. self is only the managed binary when podcli is run from inside BinDir(). If someone runs a podcli binary from elsewhere (a copied/dev build) and a /usr/local/bin/podcli or ~/.local/bin/podcli symlink points at that path, uninstall removes a link the installer never created. Low-likelihood but it's silent data loss, so worth the guard. CodeRabbit's fix is correct:
managed := filepath.Join(paths.BinDir(), "podcli"+paths.ExeSuffix())
if !pathContains(paths.BinDir(), self) {
self = "" // only unlink the managed binary, never a user-placed podcli
}
targets := uninstallTargets(home, purge)
links := podcliLinks(managed, self)and in podcliLinks:
if linkPointsTo(p, managed) || (self != "" && linkPointsTo(p, self)) {This composes cleanly with the removal loop: when self == "", pathContains(p, self) is false everywhere, which is correct — if you're not running from BinDir(), removing BinDir() can't hit your running binary.
B. Real RemoveAll errors are swallowed — valid, but mind the UX
Because self lives under BinDir() (and under home in --purge), any RemoveAll failure there — a permission error, an unrelated locked file — currently gets reclassified as runningInUse, and Done — podcli app files were removed. still prints. The error should be surfaced:
for _, p := range targets {
if err := os.RemoveAll(p); err != nil {
if pathContains(p, self) {
runningInUse = true
}
fmt.Fprintf(os.Stderr, " warning: could not remove %s: %v\n", p, err)
}
}One caveat on this one: in the expected Windows case (only the locked running .exe is left), the user now sees both the raw warning: could not remove … and the friendly "running binary is still in use" note. If you'd rather keep the clean single-message path for that case, the more precise version is to only warn when something other than the running binary remains — but that's more code, and always-warning is a reasonable quick win. Your call on the trade-off.
Neither is a blocker; A is the one I'd definitely take.
|
I have it almost done I'll be back and commit it over to the PR branch! |
podcliLinks removed any link pointing at the running executable even when it lived outside the managed bin dir, contradicting its test. Apply the outside-managed-bin guard inside the function so it is safe regardless of caller.
3 participants
Summary
This PR adds a safe, user-friendly uninstall path for podcli.
Users can now run podcli uninstall to remove podcli’s managed app files while preserving their user data by default, including config, knowledge, presets, assets, history, and cache. The command supports --dry-run, --yes, and --purge for previewing, scripting, or fully removing the managed podcli folder.
What Changed
Added a new launcher command:
podcli uninstall
podcli uninstall --dry-run
podcli uninstall --yes
podcli uninstall --purge
The command is wired into the native launcher dispatch and help output.
Added uninstall behavior that:
Shows exactly what will be removed.
Prompts before deleting unless --yes is provided.
Keeps user data by default.
Uses --purge to remove the entire managed podcli directory.
Removes installer-created links only when they point to the managed podcli binary.
Added uninstall help text for the new command.
Added macOS/Linux installer support for uninstalling via:
curl -fsSL https://raw.githubusercontent.com/nmbrthirteen/podcli/main/install.sh | sh -s -- --uninstall
This removes app files and installer-created symlinks while preserving user data.
Added Windows installer support for uninstalling via:
& ([scriptblock]::Create((irm https://raw.githubusercontent.com/nmbrthirteen/podcli/main/install.ps1))) -Uninstall
This removes podcli app files and cleans the user PATH entry while preserving user data.
Documented uninstall behavior in both the main README and npm README.
Added tests for:
Absolute and relative symlink detection.
Preserving user data by default.
Purge-mode target selection.
Testing
✅ git status --short && git log -1 --oneline
✅ nl -ba cli/main.go | sed -n '35,45p;318,454p;554,560p'
✅ nl -ba install.sh | sed -n '1,42p'
✅ nl -ba install.ps1 | sed -n '1,27p'
✅ nl -ba README.md | sed -n '182,188p'
✅ nl -ba npm/README.md | sed -n '20,21p'
✅ nl -ba cli/uninstall_test.go | sed -n '9,43p'
Summary by CodeRabbit
podcli uninstallwith interactive confirmation,--dry-run, and--purge.--purgedeletes the entire managed folder.