feat(uninstall): add granular uninstall flow on current main#241
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a first-class managed uninstall feature across the TUI and CLI, backed by a new uninstall service that plans removals, snapshots affected files, and reports manual cleanup steps. This closes the lifecycle gap between install/sync and safe reversal, including SDD profile-agent cleanup for OpenCode.
Changes:
- Introduces multi-mode uninstall flow in the TUI (partial/full/full-remove/clean-install) with confirmation + results rendering.
- Adds CLI
uninstallcommand, output reporting, and app-layer wiring. - Implements uninstall planning/execution with pre-uninstall snapshots + cleanup utilities; extends backup manifest metadata and hardens atomic writes.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/screens/uninstall.go | New uninstall mode/selection/confirm/result screen renderers. |
| internal/tui/screens/uninstall_result_test.go | Verifies uninstall result rendering includes manual cleanup section. |
| internal/tui/router.go | Adds linear routes for uninstall screens. |
| internal/tui/model.go | Adds uninstall state, navigation, execution (incl. full-remove + clean-install behavior). |
| internal/tui/model_test.go | Adds coverage for uninstall navigation and execution paths. |
| internal/model/types.go | Introduces UninstallMode enum. |
| internal/components/uninstall/service.go | New uninstall service: plan building, snapshotting, cleanup operations, and state updates. |
| internal/components/uninstall/service_test.go | Tests for manual-action reporting and SDD profile agent cleanup. |
| internal/components/uninstall/cleaners.go | Cleanup helpers for markdown/JSON/TOML and managed-file safety checks. |
| internal/components/uninstall/cleaners_test.go | Broad unit coverage for cleaners and managed-file constraints. |
| internal/components/filemerge/writer.go | Hardens atomic writes (symlink/size checks, fsync temp + directory). |
| internal/components/filemerge/writer_test.go | Updates expectations and adds tests for new hardening behaviors. |
| internal/cli/uninstall.go | Adds CLI uninstall flag parsing, confirmation prompt, execution, and reporting. |
| internal/cli/command_output_test.go | Verifies uninstall report output includes manual cleanup section. |
| internal/backup/manifest.go | Adds BackupSourceUninstall metadata. |
| internal/backup/manifest_test.go | Tests uninstall source label/display behavior. |
| internal/app/app.go | Wires gentle-ai uninstall command through app entrypoint. |
| docs/usage.md | Documents TUI uninstall flow and CLI uninstall usage/flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case ScreenModelConfig: | ||
| return len(screens.ModelConfigOptions()) | ||
| case ScreenUninstallMode: | ||
| return len(screens.UninstallModeOptions()) + 2 |
There was a problem hiding this comment.
optionCount() for ScreenUninstallMode returns len(UninstallModeOptions()) + 2, but the mode screen renders only one extra option ("Back") and confirmSelection() only handles cursor == len(options) for back. This allows the cursor to land on a non-existent option (and enter will no-op). Adjust the count to match the rendered options (likely + 1).
| return len(screens.UninstallModeOptions()) + 2 | |
| return len(screens.UninstallModeOptions()) + 1 |
| // If FullRemove mode, attempt to delete the binary itself | ||
| if mode == model.UninstallModeFullRemove { | ||
| execPath, execErr := os.Executable() | ||
| if execErr != nil { | ||
| return UninstallDoneMsg{Result: result, Err: fmt.Errorf("uninstall succeeded but failed to locate binary: %w", execErr)} | ||
| } | ||
| if removeErr := os.Remove(execPath); removeErr != nil { | ||
| return UninstallDoneMsg{Result: result, Err: fmt.Errorf("uninstall succeeded but failed to remove binary at %q: %w", execPath, removeErr)} | ||
| } |
There was a problem hiding this comment.
In FullRemove mode, attempting to os.Remove(os.Executable()) will reliably fail on Windows (running executables are locked) and can also fail on some Unix setups depending on install location/permissions. Treating this as a hard error makes the UI report an overall uninstall failure even though the managed uninstall already succeeded. Consider reporting this as a manual action (or a separate non-fatal field) so the uninstall result stays successful while still telling the user how to remove the binary.
| // If FullRemove mode, attempt to delete the binary itself | |
| if mode == model.UninstallModeFullRemove { | |
| execPath, execErr := os.Executable() | |
| if execErr != nil { | |
| return UninstallDoneMsg{Result: result, Err: fmt.Errorf("uninstall succeeded but failed to locate binary: %w", execErr)} | |
| } | |
| if removeErr := os.Remove(execPath); removeErr != nil { | |
| return UninstallDoneMsg{Result: result, Err: fmt.Errorf("uninstall succeeded but failed to remove binary at %q: %w", execPath, removeErr)} | |
| } | |
| // If FullRemove mode, attempt to delete the binary itself. | |
| // This is best-effort because removing the currently running executable | |
| // reliably fails on Windows and may fail on some Unix setups depending | |
| // on permissions or install location. The managed uninstall has already | |
| // succeeded, so do not turn self-removal failures into an overall error. | |
| if mode == model.UninstallModeFullRemove { | |
| if execPath, execErr := os.Executable(); execErr == nil { | |
| _ = os.Remove(execPath) | |
| } |
| func removeFile(path string) operation { | ||
| return operation{ | ||
| typeID: opRemoveFile, | ||
| path: path, | ||
| apply: func(path string) (bool, bool, error) { | ||
| _, statErr := os.Stat(path) | ||
| if statErr != nil { | ||
| if os.IsNotExist(statErr) { | ||
| return false, false, nil | ||
| } | ||
| return false, false, statErr | ||
| } |
There was a problem hiding this comment.
removeFile uses os.Stat (follows symlinks) to decide existence. If a managed path has been replaced with a broken symlink, os.Stat returns not-exist and the uninstall will skip removing the symlink, leaving the managed path behind. Using os.Lstat here would allow removing broken symlinks as well and avoids following links during the existence check.
| func removeTree(path string) operation { | ||
| return operation{ | ||
| typeID: opRemoveTree, | ||
| path: path, | ||
| apply: func(path string) (bool, bool, error) { | ||
| if _, err := os.Stat(path); err != nil { | ||
| if os.IsNotExist(err) { | ||
| return false, false, nil | ||
| } | ||
| return false, false, err | ||
| } |
There was a problem hiding this comment.
removeTree checks existence with os.Stat, so a broken symlink at path will be treated as "does not exist" and skipped, leaving the symlink behind. Switching the pre-check to os.Lstat would allow os.RemoveAll to remove the symlink itself even when it’s dangling, making uninstall more complete and avoiding following symlinks during the check.
|
Update from latest push (521454e):\n\n- Fixed a real runtime gap in the TUI uninstall flow by wiring in app startup and exposing a reachable Managed uninstall option from Welcome -> Uninstall Mode.\n- Hardened SDD cleanup coverage by including in managed uninstall enumerations, so SDD removals do not leave onboard artifacts behind.\n- Added regression tests for welcome navigation (with/without profiles) and updated uninstall cleanup assertions for onboard variants.\n\nValidation run locally:\n- ok github.com/gentleman-programming/gentle-ai/internal/tui (cached)\n- ok github.com/gentleman-programming/gentle-ai/internal/tui (cached)\n- ok github.com/gentleman-programming/gentle-ai/internal/tui/screens (cached)\n- ok github.com/gentleman-programming/gentle-ai/internal/app 10.399s |
|
Update from latest push (521454e):
Validation run locally:
This push is focused on closing the Judgment Day findings and the follow-up suspect navigation path. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Review: feat(uninstall): add granular uninstall flow
Solid feature PR. Clean architecture that follows the existing codebase patterns well — operation planning separated from execution, proper backup-before-mutate flow, and meaningful test coverage across all layers.
Must address
-
Breaking behavior change in
WriteFileAtomic: The removal ofos.Chmod(dir, 0o755)means directories created with0o555by previous installer versions will now cause failures. This is a security improvement but should be called out in the PR description / release notes since it affectssyncandinstalltoo, not just uninstall. -
FullRemove+ Homebrew: When the binary was installed viabrew install,os.Remove(execPath)leaves a dangling symlink in the bin path. Consider detecting Homebrew-managed installs and warning the user to usebrew uninstallinstead, or at least adding it toManualActions.
Minor suggestions
-
ScreenUninstallModeoption count uses+2but only renders 1 extra item ("Back"). The cursor can move past the visible list. -
appendPathSectioncallsos.Getwd()per-path inside the loop — easy to hoist. -
sddSkillIDs()andsddPhaseAgentsare near-duplicates — consider sharing. -
The JSON normalizer (
stripJSONComments/stripTrailingCommas) would benefit from fuzz testing given its complexity.
What I liked
- Backup-before-mutate is exactly right for destructive operations
- The
mergeRewriteOpspattern for composing multiple mutations on the same file is elegant - Good use of
readManagedFilewith size limits and symlink rejection as defense-in-depth - Test coverage is meaningful — not just "it doesn't crash" but verifying preservation of user content
- Clean Install mode (uninstall + sync) is a great UX addition for fixing broken configs
Overall this is well-crafted work. The items above are refinements, not blockers. Leaning approve once #1 and #2 are addressed or acknowledged.
|
Gracias por el review. Ya resolví los puntos de Alan Buscaglia y dejé el trabajo dividido en commits separados para que sea más fácil de revisar:
Además actualicé la PR para dejar claro el issue linkeado y el tipo de cambio. Si querés, puedo seguir con cualquier ajuste adicional que veas. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Re-review — both items addressed
Item 1: WriteFileAtomic breaking behavior on 0o555 dirs
Fixed. The old unconditional os.Chmod(dir, 0o755) is replaced by ensureAtomicParentDir which:
- Only relaxes permissions when the owner-write bit is actually missing (
perm & 0o200 == 0) - Creates new directories with
0o700(tighter default) - Rejects symlink parent directories via
os.Lstatcheck - Test renamed to
TestWriteFileAtomicReadOnlyDirRelaxesOwnerWritePermission— accurately describes the behavior
Bonus: symlink rejection for both target files (readComparableFile) and parent dirs, with tests. Good hardening.
Item 2: FullRemove + Homebrew dangling symlink
Fixed. isHomebrewManagedBinary detects Cellar, /opt/homebrew/, /usr/local/Homebrew/, and linuxbrew paths. When matched, os.Remove is skipped and a manual action is surfaced: "Run 'brew uninstall gentle-ai' to remove the executable cleanly." Two tests cover both the brew and non-brew paths.
Security audit (quick pass)
WriteFileAtomic:tmp.Sync()+ dirSync()added — crash-safe atomicity. Symlink checks on file and parent. Max file size guard on reads. All good.readManagedFilein cleaners: same Lstat + size guard pattern. Consistent.removeTree: usesos.Stat(follows symlinks) beforeos.RemoveAll. If a managed path were ever a symlink, this would follow through and delete the real target. Non-blocking since paths are constructed from constants, but switching toos.Lstatwould be more defensive. Consider for a follow-up.osRemoveFn/osExecutableFntestability hooks: clean pattern, no production risk.
Verdict
LGTM — both review items resolved, security posture improved. Ship it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| opts = append(opts, "Manage backups") | ||
| opts = append(opts, "Managed uninstall") | ||
| opts = append(opts, "Quit") |
| profileSelectionUsed := m.UninstallProfileSelection || len(profileNamesToRemove) > 0 | ||
| mode := m.UninstallMode | ||
| return func() tea.Msg { | ||
| if uninstallFn == nil && uninstallWithProfilesFn == nil { | ||
| return UninstallDoneMsg{Err: fmt.Errorf("uninstall function not configured")} | ||
| } | ||
|
|
||
| var ( | ||
| result componentuninstall.Result | ||
| err error | ||
| ) | ||
| if uninstallWithProfilesFn != nil && profileSelectionUsed { | ||
| result, err = uninstallWithProfilesFn(agentIDs, componentIDs, profileNamesToRemove, engramScope) | ||
| } else { | ||
| result, err = uninstallFn(agentIDs, componentIDs) | ||
| } |
| } | ||
| return m | ||
| } | ||
|
|
| if (mode == model.UninstallModeFull || mode == model.UninstallModeFullRemove) || hasWorkspaceAssets { | ||
| b.WriteString(styles.WarningStyle.Render("⚠ Workspace Assets Warning:")) | ||
| b.WriteString("\n") | ||
| b.WriteString(styles.SubtextStyle.Render(" Removing SDD or Skills will delete workspace-scoped files like:")) | ||
| b.WriteString("\n") | ||
| b.WriteString(styles.SubtextStyle.Render(" • .windsurf/workflows/ (SDD workflows)")) | ||
| b.WriteString("\n") | ||
| b.WriteString(styles.SubtextStyle.Render(" • .engram/ (persistent memory context)")) | ||
| b.WriteString("\n") |
- Add ScreenUninstallMode as entry point for uninstall flow - Three modes: Partial (granular selection), Full (all agents/components), Full & Remove (includes binary deletion) - Workspace asset warnings for .engram/, workflows/, and skills/ - Full & Remove mode uses os.Remove() to delete gentle-ai binary after successful uninstall - Updated tests to reflect new navigation flow through mode selector - Closes part of Gentleman-Programming#102
- Extend uninstall result with ManualActions - Report directories left non-empty after managed cleanup - Show manual cleanup section in CLI and TUI uninstall results - Add tests for service, CLI report, and TUI result rendering
Adds a 4th uninstall mode that removes all managed configuration and immediately re-syncs all managed assets from scratch. This provides a factory-reset workflow to fix broken configurations without manually re-running install/sync commands. The sync runs after uninstall completes. Sync errors are non-fatal and displayed separately in the result screen so the user always sees the uninstall status.
Ensure managed uninstall is reachable from the Welcome menu and correctly wired to the backend callback, and include sdd-onboard in SDD cleanup lists to avoid leftover managed artifacts.
…writes Restore WriteFileAtomic permission-relaxation compatibility, prevent Homebrew full-remove from deleting managed binaries, and tighten uninstall cleanup safety checks so reviewer-reported edge cases are handled explicitly.
Add a conditional profile-selection step so users can choose which OpenCode SDD profiles are removed, while preserving the old flow when no profiles apply.
…g#265 state.Write API change - Adapt uninstall service to use state.InstallState instead of []string - Update app.go dispatch to match RunUninstall return signature - Replace incompatible uninstall tests with PR-compatible version
Alan-TheGentleman
force-pushed
the
feat/issue-102-clean
branch
from
fcdf7c0 to
f172888
Compare
Alan-TheGentleman
merged commit 4d03aca
into
Gentleman-Programming:main
🔗 Linked Issue
Closes #102
🏷️ PR Type
What kind of change does this PR introduce?
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)📝 Summary
mainwithout the unrelated branch drift that affected the previous PR.📂 Changes
internal/tui/model.gointernal/tui/router.gointernal/tui/screens/uninstall.gointernal/app/app.gointernal/cli/uninstall.gointernal/components/uninstall/*internal/components/filemerge/*internal/backup/manifest.godocs/usage.md🧪 Test Plan
Unit Tests
go test ./internal/components/uninstall ./internal/backup ./internal/tui ./internal/app ./internal/cliAdditional validation
go test ./internal/components/sdd ./internal/model🤖 Automated Checks
The following checks run automatically on this PR:
Closes/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*label must be applied✅ Contributor Checklist
status:approvedtype:*label to this PRCo-Authored-Bytrailers💬 Notes for Reviewers
This PR replaces the previous uninstall PR with a clean branch based on current
main.The prior PR included unrelated drift because the original work had been started from an old base. This branch was rebuilt on top of today’s
main, preserving the uninstall logic and adapting it to current repository state.One important adaptation versus the original implementation: uninstall now also removes current OpenCode SDD profile agent entries (
sdd-*-<profile>) when uninstalling the SDD component, while leaving unrelated user settings intact.