fix: improve uninstall wizard cleanup#147
Open
udhaya10 wants to merge 2 commits intoparcadei:mainfrom
Open
Conversation
- Add settings.local.json to PRESERVE_FILES (user permissions) - Log warnings instead of silently ignoring preserve failures - Remove CLAUDE_OPC_DIR from shell config (~/.zshrc, ~/.bashrc) - Offer to remove TLDR CLI tool (uv tool uninstall llm-tldr) Closes parcadei#142
📝 WalkthroughWalkthroughThis pull request enhances the uninstall process by preserving Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
PR author is not in the allowed authors list. |
![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
🤖 Fix all issues with AI agents
In `@opc/scripts/setup/wizard.py`:
- Around line 1352-1366: The uninstall call using subprocess.run inside the TLDR
removal block should be wrapped in a try/except to handle
subprocess.TimeoutExpired and OSError/FileNotFoundError so the wizard doesn't
crash; locate the block that checks shutil.which("tldr") and Confirm.ask, then
around the subprocess.run call (which sets timeout=30) catch
subprocess.TimeoutExpired and OSError/FileNotFoundError (or the base OSError)
and log a warning via console.print including guidance to run "uv tool uninstall
llm-tldr" manually, and also handle non-zero return codes as currently done
using remove_result.returncode.
🧹 Nitpick comments (1)
opc/scripts/setup/wizard.py (1)
1352-1353:shutil.which("tldr")doesn't distinguishllm-tldrfromtldr-pages.The install wizard (Line 960) validates this by checking for
"tree","structure","daemon"in--helpoutput. Here, offering touv tool uninstall llm-tldrwhen the detectedtldris actuallytldr-pagesis harmless (uv will report it's not installed), but could confuse the user. Consider either skipping the check or adding a similar heuristic.
Comment on lines
+1352
to
+1366
| if shutil.which("tldr"): | ||
| if Confirm.ask("\n Remove TLDR CLI tool?", default=False): | ||
| import subprocess | ||
|
|
||
| remove_result = subprocess.run( | ||
| ["uv", "tool", "uninstall", "llm-tldr"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| if remove_result.returncode == 0: | ||
| console.print(" [green]OK[/green] TLDR removed") | ||
| else: | ||
| console.print(" [yellow]WARN[/yellow] Could not remove TLDR") | ||
| console.print(" Remove manually: uv tool uninstall llm-tldr") |
There was a problem hiding this comment.
Missing error handling for subprocess.run — TimeoutExpired and OSError can propagate uncaught.
If uv is not on PATH or the process times out (Line 1360 sets timeout=30), the resulting FileNotFoundError / subprocess.TimeoutExpired will bubble up unhandled and crash the wizard after an otherwise successful uninstall.
🛡️ Proposed fix: wrap in try/except
if shutil.which("tldr"):
if Confirm.ask("\n Remove TLDR CLI tool?", default=False):
import subprocess
- remove_result = subprocess.run(
- ["uv", "tool", "uninstall", "llm-tldr"],
- capture_output=True,
- text=True,
- timeout=30,
- )
- if remove_result.returncode == 0:
- console.print(" [green]OK[/green] TLDR removed")
- else:
- console.print(" [yellow]WARN[/yellow] Could not remove TLDR")
+ try:
+ remove_result = subprocess.run(
+ ["uv", "tool", "uninstall", "llm-tldr"],
+ capture_output=True,
+ text=True,
+ timeout=30,
+ )
+ if remove_result.returncode == 0:
+ console.print(" [green]OK[/green] TLDR removed")
+ else:
+ console.print(" [yellow]WARN[/yellow] Could not remove TLDR")
+ console.print(" Remove manually: uv tool uninstall llm-tldr")
+ except (subprocess.TimeoutExpired, OSError) as e:
+ console.print(f" [yellow]WARN[/yellow] Could not remove TLDR: {e}")
console.print(" Remove manually: uv tool uninstall llm-tldr")🤖 Prompt for AI Agents
In `@opc/scripts/setup/wizard.py` around lines 1352 - 1366, The uninstall call
using subprocess.run inside the TLDR removal block should be wrapped in a
try/except to handle subprocess.TimeoutExpired and OSError/FileNotFoundError so
the wizard doesn't crash; locate the block that checks shutil.which("tldr") and
Confirm.ask, then around the subprocess.run call (which sets timeout=30) catch
subprocess.TimeoutExpired and OSError/FileNotFoundError (or the base OSError)
and log a warning via console.print including guidance to run "uv tool uninstall
llm-tldr" manually, and also handle non-zero return codes as currently done
using remove_result.returncode.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
opc/scripts/setup/wizard.py
Outdated
Comment on lines
+1358
to
+1361
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Wrap subprocess.run() in try/except to catch TimeoutExpired and OSError, preventing the uninstall wizard from crashing if uv is missing or times out. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
The uninstall wizard archives
~/.claudeand restores backup, but leaves several artifacts behind. This PR fixes that.Changes
claude_integration.py
settings.local.jsontoPRESERVE_FILES— users store custom permissions here; was being lost on uninstallexcept Exception: pass, now records which files failed to preserve and includes them in the summary messagewizard.py
CLAUDE_OPC_DIRfrom shell config — the install wizard addsexport CLAUDE_OPC_DIR=...to~/.zshrc/~/.bashrcbut uninstall never cleaned it upuv tool uninstall llm-tldr(default: No, since user might want to keep it)Not included (intentionally)
Test plan
--uninstallwithCLAUDE_OPC_DIRin~/.zshrc→ verify it's removed--uninstallwithsettings.local.jsonpresent → verify it's preserved--uninstallwith TLDR installed → verify removal prompt appearsCloses #142
Summary by CodeRabbit
New Features
Bug Fixes