Make uninstall remove all managed data#41
Conversation
📝 WalkthroughWalkthroughThe uninstall flow now treats ChangesUninstall and installer behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Status text normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
teethatkamsai
force-pushed
the
codex/uninstall-cleans-all
branch
from
94a00af to
5e13098
Compare
|
â is mojibake: the terminal decoded a Unicode em dash (—) with the wrong encoding, so Done — run: podcli displayed as Done â run: podcli. I made the small patch by replacing that output with plain ASCII: I also changed the related uninstall messages to use - instead of Unicode punctuation, so PowerShell/terminal encoding won’t garble them. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.ps1 (1)
16-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign PowerShell uninstall with the CLI default —
podcli uninstallremoves the full managed home by default, butinstall.ps1 -Uninstallonly deletesbin/runtime/models/toolsunless-Purgeis passed. That makes the same uninstall action behave differently across entry points; if parity is intended, default the PowerShell path to$homeDirand keep-Purgeas compatibility only.🤖 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` around lines 16 - 42, The uninstall flow in install.ps1 is inconsistent with the CLI default because the $Purge branch only controls deletion of $homeDir while the non-Purge path currently keeps user data by deleting only bin/runtime/models/tools. Update the uninstall logic in the same block that builds $targets so that the default -Uninstall behavior matches podcli uninstall by removing the full managed home via $homeDir, and treat -Purge as a compatibility option rather than the default behavior.
🤖 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 423-441: The removeFromWindowsUserPath logic rewrites the user
Path with SetEnvironmentVariable(..., 'User'), which can change expandable
entries like %SOMEVAR%\bin into a non-expandable value. Update the PowerShell
path editing in removeFromWindowsUserPath to preserve the original registry
value type instead of rewriting as REG_SZ, and make the matching change in
install.ps1 so both PATH update paths handle expandable entries consistently.
---
Outside diff comments:
In `@install.ps1`:
- Around line 16-42: The uninstall flow in install.ps1 is inconsistent with the
CLI default because the $Purge branch only controls deletion of $homeDir while
the non-Purge path currently keeps user data by deleting only
bin/runtime/models/tools. Update the uninstall logic in the same block that
builds $targets so that the default -Uninstall behavior matches podcli uninstall
by removing the full managed home via $homeDir, and treat -Purge as a
compatibility option rather than the default behavior.
🪄 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: 64f103d0-9a0b-4a09-9665-dac11ae514ba
📒 Files selected for processing (4)
cli/main.gocli/uninstall_test.goinstall.ps1install.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.ps1 (1)
35-66: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign
install.ps1 -Uninstallwithpodcli uninstall
podcli uninstallalready removes the full managed folder and treats--purgeas compatibility-only, but this script still keepsruntime,models,tools, and user data unless-Purgeis passed. Make the default path remove$homeDirso both entry points behave the same.🤖 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` around lines 35 - 66, The uninstall flow in install.ps1 currently diverges from podcli uninstall by only deleting binDir and leaving runtime, models, tools, and user data unless -Purge is set. Update the uninstall branch in the script’s Uninstall/Purge handling so the default target set removes the full managed $homeDir, matching podcli uninstall behavior, and keep -Purge only as compatibility behavior if needed. Ensure the PATH cleanup and status messages still use Get-UserPathEntry, Test-PathEntryEquals, and the existing removal loop.
🧹 Nitpick comments (2)
install.ps1 (2)
14-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty catch block silently swallows registry errors.
Static analysis flags the empty
catch {}at Line 18. While falling back toExpandStringon failure to read the value kind is reasonable, silently swallowing the exception makes future debugging (e.g. permission errors) harder.🔧 Suggested fix
$kind = [Microsoft.Win32.RegistryValueKind]::ExpandString - try { $kind = $key.GetValueKind('Path') } catch {} + try { $kind = $key.GetValueKind('Path') } catch { Write-Verbose "Could not read Path value kind: $($_.Exception.Message)" }🤖 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` around lines 14 - 24, The empty catch in Get-UserPathEntry is swallowing registry lookup errors, making failures hard to diagnose. Update the try/catch around $key.GetValueKind('Path') so the exception is handled explicitly by logging, annotating, or otherwise preserving the error details while still falling back to Microsoft.Win32.RegistryValueKind.ExpandString when the kind cannot be read. Keep the change localized to Get-UserPathEntry and preserve the existing return shape with Key, Kind, and Value.Source: Linters/SAST tools
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefensive
byte[]check on.Contentis likely dead code but harmless.
Invoke-WebRequest -UseBasicParsingnormally returns.Contentas a[string]for text/plain responses likechecksums.txt; the-is [byte[]]branch is unlikely to trigger in practice, though it's a harmless safety net.🤖 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` around lines 85 - 89, The `$sums` handling in the `try` block uses a defensive `.Content -is [byte[]]` branch that is effectively dead code for `Invoke-WebRequest` responses from `checksums.txt`. Simplify the `Invoke-WebRequest`/`$sums` logic by removing the unnecessary byte-array conversion path and keep the content handling as a plain string in `install.ps1`.
🤖 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 `@install.ps1`:
- Around line 52-60: The PATH update in the user PATH handling block is only
writing to the registry, so Explorer may keep using a stale environment block.
After each successful Path SetValue call in install.ps1, broadcast
WM_SETTINGCHANGE so new terminals launched from Explorer pick up the updated
PATH; use the existing Get-UserPathEntry / $pathEntry.Key.SetValue flow and keep
the current remove/update logic unchanged.
---
Outside diff comments:
In `@install.ps1`:
- Around line 35-66: The uninstall flow in install.ps1 currently diverges from
podcli uninstall by only deleting binDir and leaving runtime, models, tools, and
user data unless -Purge is set. Update the uninstall branch in the script’s
Uninstall/Purge handling so the default target set removes the full managed
$homeDir, matching podcli uninstall behavior, and keep -Purge only as
compatibility behavior if needed. Ensure the PATH cleanup and status messages
still use Get-UserPathEntry, Test-PathEntryEquals, and the existing removal
loop.
---
Nitpick comments:
In `@install.ps1`:
- Around line 14-24: The empty catch in Get-UserPathEntry is swallowing registry
lookup errors, making failures hard to diagnose. Update the try/catch around
$key.GetValueKind('Path') so the exception is handled explicitly by logging,
annotating, or otherwise preserving the error details while still falling back
to Microsoft.Win32.RegistryValueKind.ExpandString when the kind cannot be read.
Keep the change localized to Get-UserPathEntry and preserve the existing return
shape with Key, Kind, and Value.
- Around line 85-89: The `$sums` handling in the `try` block uses a defensive
`.Content -is [byte[]]` branch that is effectively dead code for
`Invoke-WebRequest` responses from `checksums.txt`. Simplify the
`Invoke-WebRequest`/`$sums` logic by removing the unnecessary byte-array
conversion path and keep the content handling as a plain string in
`install.ps1`.
🪄 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: 98a9698f-4998-41fd-af9f-05e83c7b6757
📒 Files selected for processing (7)
backend/cli.pybackend/utils/proc.pycli/internal/provision/provision.gocli/internal/update/update.gocli/main.goinstall.ps1install.sh
✅ Files skipped from review due to trivial changes (4)
- backend/cli.py
- cli/internal/update/update.go
- install.sh
- cli/internal/provision/provision.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/main.go
|
CodeRabbit follow-up (install.ps1:60, WM_SETTINGCHANGE): deferring as a minor. The important fix landed — PATH is written with its original registry type preserved ( |
2 participants
Summary
Changes
podcli uninstallto fully remove the managed podcli installation by default, including runtime files, models, cached data, and the managed user data folder.Changes
podcli uninstallremove the full managed podcli folder by default.--purgeaccepted as a compatibility alias.%LOCALAPPDATA%\podcli\binfrom the Windows user PATH during uninstall.--dry-run.install.ps1 -Uninstall -Purgesupport and keep PATH cleanup there too.Validation
install.ps1parses cleanly.go test ./internal/paths ./internal/updatego test ./...was attempted but is blocked by existing missing embedded assets and an unrelated provision symlink test.Summary by CodeRabbit
--purgeremains for compatibility.