Skip to content

cli, app: Complete uninstall cleanup (#1843) - #1845

Merged
nortonandreev merged 3 commits into
mainfrom
norton/uninstall-followups
Oct 22, 2026
Merged

nortonandreev merged 3 commits into
mainfrom
norton/uninstall-followups

Conversation

@nortonandreev

@nortonandreev nortonandreev commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1843 — AI-4248

What & why

Full uninstall missed several integration and updater files, and could delete the local lifecycle lock and retired-URL registry with the config directory.

  • Once a local setup has created its root, cleanup holds the lifecycle lease through integration and config removal, keeps lifecycle.lock and install.json (no recordings), and rechecks consent under it: a server that appears or changes after confirmation stops the uninstall, --keep-config included, unless --purge-local. With no root, ~/.config/kcap is deleted whole.
  • An unreadable registry leaves the local server and config dir but still removes integrations, exiting 1.
  • Adds Mistral Vibe hooks/MCP, ~/.cache/kcap, legacy Cursor spool, retired Kiro/Crew/Antigravity skills, and the macOS app's own Velopack cache, scratch dir and logs.
  • Malformed Antigravity hooks, unreadable Kiro settings and a git config git cannot edit count as incomplete; with no runnable git the hook is skipped. A Kiro default still naming a missing kcap agent is reset.

Where to look

UninstallCommand.HandleAsync: the lease is skipped when the root is absent just before it would be taken, so a setup started during the prompt still meets the consent checks.

Verification

Check Result
CLI unit suite 7668 passed, 2 failed (Finds_the_PR_by_the_tracked_remote_name…, A_later_hit_is_a_repository_change…; both pass alone)
Core / App unit suites 5075 / 4168 passed, 0 failed
AOT publish osx-arm64 0 IL warnings
Published kcap uninstall --yes, scratch HOME, no local server exit 0, ~/.config/kcap, ~/.cache/kcap, Pi extension gone, no ~/.vibe
OsxVelopackLocator on a packaged 1.4.0-beta.3 bundle PackagesDir ~/Library/Caches/velopack/kcap/packages, AppTempDir $TMPDIR/velopack/kcap; test fixture matches

Not run: uninstall against the installed app or a real local server.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 33 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 7b49f180-18f9-4dfd-a8c2-2b508d6f9e03
📥 Commits

Reviewing files that changed from the base of the PR and between 06fa338 and 1606fcd.

📒 Files selected for processing: 18
  • README.md
  • src/Capacitor.App/Platform/Mac/MacAppRemoval.cs
  • src/Capacitor.App/Platform/Mac/MacUpdaterFiles.cs
  • src/Capacitor.Cli.Core/Harness/Antigravity/AntigravityHooksInstaller.cs
  • src/Capacitor.Cli.Core/Harness/Kiro/KiroSettings.cs
  • src/Capacitor.Cli.Core/Harness/MistralVibe/MistralVibeConfigToml.cs
  • src/Capacitor.Cli.Core/Harness/MistralVibe/MistralVibeHooksInstaller.cs
  • src/Capacitor.Cli.Core/LocalEdition/LocalLifecycle.cs
  • src/Capacitor.Cli.Core/Resources/help-uninstall.txt
  • src/Capacitor.Cli/Commands/PluginCommand.cs
  • src/Capacitor.Cli/Commands/UninstallCommand.cs
  • src/Capacitor.Cli/Commands/UninstallFiles.cs
  • src/Capacitor.Cli/GitHookInstaller.cs
  • src/Capacitor.Cli/LocalEdition/LocalStack.cs
  • test/Capacitor.App.Tests.Unit/MacAppRemovalTests.cs
  • test/Capacitor.Cli.Tests.Unit/Commands/UninstallCommandTests.cs
  • test/Capacitor.Cli.Tests.Unit/Commands/UninstallFilesTests.cs
  • test/Capacitor.Cli.Tests.Unit/GitHookInstallerTests.cs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Complete uninstall cleanup without losing local lifecycle safeguards

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Hold the local lifecycle lease through cleanup and recheck data-deletion consent before recovery.
• Remove missed integrations, legacy skills, caches, recordings, and this macOS app’s updater files.
• Report incomplete cleanup and retain the registry, lock, and retry information needed for safe
 reruns.
Diagram

graph TD
  CLI["Uninstall command"] --> Lease["Lifecycle lease"] --> Consent{"Consent valid?"} --> Recovery["Recover local state"] --> Server["Remove local server"] --> Integrations["Unwire integrations"] --> Config["Clean configuration"] --> Safety["Retain safety metadata"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move lifecycle metadata outside the config directory
  • ➕ Would allow whole-directory configuration deletion without selective traversal.
  • ➖ Requires migrating existing installations and coordinating old and new lock locations; migration itself creates safety risk.

Recommendation: Retaining the existing local root and selectively deleting configuration is the better approach for this fix: it preserves established lock and retired-URL locations without a risky migration. The alternative is worth considering only as a separately planned storage-layout change.

Files changed (15) +575 / -226

Enhancement (2) +37 / -8
MacAppRemoval.csInclude owned updater files in macOS support cleanup +13/-8

Include owned updater files in macOS support cleanup

• Deletes resolved Velopack directories and logs alongside bundle support files, while reporting removal errors.

src/Capacitor.App/Platform/Mac/MacAppRemoval.cs

MacUpdaterFiles.csResolve updater files belonging to the current bundle +24/-0

Resolve updater files belonging to the current bundle

• Uses the Velopack locator to identify cache and scratch directories and log paths, subject to bundle and app-ID checks.

src/Capacitor.App/Platform/Mac/MacUpdaterFiles.cs

Bug fix (8) +301 / -213
AntigravityHooksInstaller.csSurface failed Antigravity hook removal +14/-24

Surface failed Antigravity hook removal

• Rejects non-object or malformed hooks rather than silently treating them as removed. Propagates file-deletion failures and preserves the plugin manifest while hooks remain.

src/Capacitor.Cli.Core/Harness/Antigravity/AntigravityHooksInstaller.cs

KiroSettings.csAdd strict reading of Kiro’s default agent +12/-0

Add strict reading of Kiro’s default agent

• Distinguishes missing settings from unreadable or malformed settings so removal can retain the agent and its previous-default marker for retry.

src/Capacitor.Cli.Core/Harness/Kiro/KiroSettings.cs

LocalLifecycle.csAllow a pre-recovery check under the lifecycle lease +4/-1

Allow a pre-recovery check under the lifecycle lease

• Adds an optional callback after lease acquisition but before interrupted-operation recovery, enabling consent checks before recovery mutates or deletes data.

src/Capacitor.Cli.Core/LocalEdition/LocalLifecycle.cs

PluginCommand.csPreserve Kiro restoration state when settings are unreadable +3/-1

Preserve Kiro restoration state when settings are unreadable

• Reads the current default agent strictly when a kcap agent may exist, allowing removal to fail rather than discard retry information.

src/Capacitor.Cli/Commands/PluginCommand.cs

UninstallCommand.csKeep uninstall locked through safe, comprehensive cleanup +230/-184

Keep uninstall locked through safe, comprehensive cleanup

• Rechecks local-data consent before recovery, retains the lease through integration and configuration cleanup, and refuses to delete orphaned local files without a registry. Adds Mistral Vibe, legacy skill, cache, and Cursor spool cleanup while tracking git-hook and integration failures.

src/Capacitor.Cli/Commands/UninstallCommand.cs

UninstallFiles.csRemove configuration without deleting the local root +28/-0

Remove configuration without deleting the local root

• Selectively deletes configuration around a nested local root, retaining its lifecycle metadata and avoiding traversal through unrelated links.

src/Capacitor.Cli/Commands/UninstallFiles.cs

GitHookInstaller.csReport unsuccessful global git-hook removal +7/-1

Report unsuccessful global git-hook removal

• Makes removal return success or failure, distinguishing an absent configuration or hook from a failed git read or edit.

src/Capacitor.Cli/GitHookInstaller.cs

LocalStack.csExpose a locked, pre-recovery local-state check +3/-2

Expose a locked, pre-recovery local-state check

• Passes an optional callback to the lifecycle runner so uninstall can inspect current state under the lease before recovery.

src/Capacitor.Cli/LocalEdition/LocalStack.cs

Tests (3) +226 / -3
MacAppRemovalTests.csTest bundle-scoped updater cleanup +29/-0

Test bundle-scoped updater cleanup

• Verifies removal of this app’s Velopack cache, scratch files, and logs while preserving another app’s cache and rejecting a mismatched bundle.

test/Capacitor.App.Tests.Unit/MacAppRemovalTests.cs

UninstallCommandTests.csCover missed cleanup and local-data safety cases +172/-3

Cover missed cleanup and local-data safety cases

• Tests new integration and legacy-file removal, retry-state retention on failures, consent when setup or purge appears after inspection, orphaned local data, and lock and registry retention through cleanup.

test/Capacitor.Cli.Tests.Unit/Commands/UninstallCommandTests.cs

UninstallFilesTests.csTest selective configuration deletion around a nested local root +25/-0

Test selective configuration deletion around a nested local root

• Confirms that configuration files are removed while lifecycle metadata and files behind an unrelated symbolic link remain untouched.

test/Capacitor.Cli.Tests.Unit/Commands/UninstallFilesTests.cs

Documentation (2) +11 / -2
README.mdDocument retained safety metadata and expanded uninstall coverage +5/-1

Document retained safety metadata and expanded uninstall coverage

• Explains why the lifecycle lock and retired-URL registry remain, how consent is rechecked, and which additional integrations and files are removed. Documents incomplete-cleanup reporting and macOS updater cleanup.

README.md

help-uninstall.txtClarify uninstall scope and retained lifecycle metadata +6/-1

Clarify uninstall scope and retained lifecycle metadata

• Describes additional cleanup targets and explains that the lock and retired-URL registry remain after configuration removal.

src/Capacitor.Cli.Core/Resources/help-uninstall.txt

@nortonandreev nortonandreev self-assigned this Oct 22, 2026
@qodo-code-review

qodo-code-review Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. A root config path risks deleting user files ✗ Dismissed
Description
DeleteConfig tests descendants using physicalPath + Path.DirectorySeparatorChar, which produces
// rather than / when the configuration directory is a filesystem root. If KCAP_CONFIG_DIR
names that root, the retained local installation does not match the test and cleanup falls through
to recursive deletion of the root.
Code

src/Capacitor.Cli/Commands/UninstallFiles.cs[17]

+        if (keep is not null && physicalKeep!.StartsWith(physicalPath + Path.DirectorySeparatorChar, comparison)) {
Relevance

●●● Strong

A recent accepted precedent specifically addresses duplicate separators when containment checks
encounter filesystem roots.

PR-#1411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
A configured directory is accepted directly from the environment. The new comparison adds a
separator even when the resolved path already ends in one, and the nonmatching branch recursively
deletes the configured directory.

src/Capacitor.Cli.Core/ConfigRoot.cs[39-47]
src/Capacitor.Cli/Commands/UninstallFiles.cs[15-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The local-root containment check fails for a filesystem-root configuration directory and permits recursive deletion of that root.
## Fix Focus Areas
- src/Capacitor.Cli/Commands/UninstallFiles.cs[15-26]
## Recommended Fix
Reject filesystem-root configuration directories before cleanup, or use a root-aware containment check that cannot fall through to recursive root deletion. Add a test with a local root beneath the configured filesystem root.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Uninstall can delete the wrong directory ✗ Dismissed
Description
DeleteConfig calls Path.GetFullPath before resolving symlinks, so it can change the meaning of a
configured path containing a symlink followed by ... If KCAP_CONFIG_DIR is
config/link/../target and link points outside config, cleanup validates and deletes
config/target rather than the directory reached through the configured path.
Code

src/Capacitor.Cli/Commands/UninstallFiles.cs[R9-10]

+        var path = Path.TrimEndingDirectorySeparator(Path.GetFullPath(directory));
+        var keep = localRoot is null ? null : Path.TrimEndingDirectorySeparator(Path.GetFullPath(localRoot));
Relevance

●●● Strong

Accepted symlink-safety precedents support resolving filesystem aliases before destructive path
validation.

PR-#191
PR-#938

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
ConfigRoot retains the environment-supplied directory, while CanonicalPath explicitly avoids
GetFullPath on rooted paths because it folds .. before following a preceding symlink. The new
cleanup does that fold first and then deletes the resulting path.

src/Capacitor.Cli.Core/ConfigRoot.cs[39-47]
src/Capacitor.Cli.Core/CanonicalPath.cs[20-25]
src/Capacitor.Cli/Commands/UninstallFiles.cs[8-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DeleteConfig` normalizes a configured path before resolving its symlinks, allowing cleanup to operate on a different directory.
## Fix Focus Areas
- src/Capacitor.Cli/Commands/UninstallFiles.cs[8-15]
- src/Capacitor.Cli/Commands/UninstallFiles.cs[20-26]
## Recommended Fix
Validate the original configured path component by component before any lexical normalization, and ensure deletion operates on the validated destination. Reject paths whose symlink-and-parent traversal cannot be handled safely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Kiro can stay pointed at a deleted agent ✗ Dismissed
Description
currentDefault is now read only when `KiroHooksInstaller.IsInstalled(agentPath) ||
File.Exists(agentPath)`, and is null otherwise. When kcap.json and its marker are already gone but
chat.defaultAgent is still kcap, the restore is skipped. The old code restored the default
whenever ReadDefaultAgent(settingsPath) == KiroAgentName, so this case used to be repaired. Now
uninstall reports success while Kiro's default still points at an agent that no longer exists.
Code

src/Capacitor.Cli/Commands/PluginCommand.cs[R2021-2023]

+            var currentDefault = KiroHooksInstaller.IsInstalled(agentPath) || File.Exists(agentPath)
+                ? KiroSettings.ReadDefaultAgentStrict(settingsPath) : null;
+            if (currentDefault == KiroAgentName
Relevance

●●● Strong

Prior Kiro cleanup feedback supports preserving default restoration and retry safety when agent
state is inconsistent.

PR-#165

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The removed line called KiroSettings.ReadDefaultAgent(settingsPath) == KiroAgentName
unconditionally. The new gate only reads settings when the agent file or marker exists, so a default
left as kcap after manual deletion of kcap.json is never reset to kiro_default.

src/Capacitor.Cli/Commands/PluginCommand.cs[2013-2041]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new gate on `ReadDefaultAgentStrict` skips restoring Kiro's default agent when kcap.json and its marker are already gone but `chat.defaultAgent` is still `kcap`.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/PluginCommand.cs[2020-2032]

## Recommended Fix
Use `ReadDefaultAgentStrict` when the agent or marker exists, so failures stay retryable. Otherwise fall back to `KiroSettings.ReadDefaultAgent(settingsPath)` rather than null. A default still set to `kcap` is then reset to `previousDefault` (`kiro_default`) even when the agent file is gone.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Temp-file tests bypass cleanup helpers ✗ Dismissed
Description
Removes_config_beside_a_nested_local_root_without_following_unrelated_links writes install.json
and lifecycle.lock with File.WriteAllText over Path.Combine paths beneath Tmp. Those files
are inside the TempDir root but bypass its creation and cleanup helpers, so the test does not
consistently use the managed temporary-filesystem abstraction.
Code

test/Capacitor.Cli.Tests.Unit/Commands/UninstallFilesTests.cs[R13-14]

+        File.WriteAllText(Path.Combine(local, "install.json"), "registry");
+        File.WriteAllText(Path.Combine(local, "lifecycle.lock"), "");
Relevance

●●● Strong

Recent accepted test reviews require temporary filesystem state to use TempDir helper APIs.

PR-#1676
PR-#1599

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test creates an injected TempDir at line 7, then directly writes files beneath that root with
File.WriteAllText(Path.Combine(...)) at lines 13-14. Compliance ID 2767472 requires all temporary
filesystem state under a TempDir to use its helper members.

Rule 2767472: Use TempDir helper for all temporary filesystem state in tests
test/Capacitor.Cli.Tests.Unit/Commands/UninstallFilesTests.cs[7-14]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test writes files beneath the injected `TempDir` using `File.WriteAllText` and manually combined paths instead of using the helper abstraction required for temporary filesystem state.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/Commands/UninstallFilesTests.cs[13-14]

## Recommended Fix
Create both files with `Tmp.CreateFile` using paths relative to the temporary root, such as `config/nested/local/install.json` and `config/nested/local/lifecycle.lock`, and keep the returned paths only where later assertions require them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Comments retain obsolete change history ✗ Dismissed
Description
CleanupAsync includes comments about manually pruned entries and a pre-marker build instead of
describing only the current marker-cleanup behavior. These historical references are encountered
whenever uninstall cleanup runs and make the rationale depend on past releases and prior edits
rather than the present invariant.
Code

src/Capacitor.Cli/Commands/UninstallCommand.cs[R271-274]

+            // marker only when JSON entries changed; if the user manually pruned the
+            // entries earlier (or installed via a pre-marker build that later
+            // wrote a marker on first refresh), the marker survives and IsInstalled
+            // still reports kcap as installed. uninstall promises a full
Relevance

●●● Strong

Recent accepted reviews consistently require concise comments without historical implementation
narratives.

PR-#360
PR-#638

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The added comments narrate historical implementation and edit context, including a pre-marker build
and manually pruned entries. This conflicts with the requirement that comments describe current
behavior, intent, invariants, or rationale without change-history narration.

Rule 2897915: Avoid time-sensitive or process-reference metadata in code comments
src/Capacitor.Cli/Commands/UninstallCommand.cs[270-276]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The marker-cleanup comments describe historical release and manual-edit scenarios, including a pre-marker build and earlier pruning, instead of documenting the current cleanup invariant.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/UninstallCommand.cs[270-276]

## Recommended Fix
Rewrite the comments to explain only the current behavior and rationale: which markers are removed, why the Cursor marker is handled by its own remover, and when retry state must be retained. Remove references to past releases, prior edits, or change history.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (4)
6. Partially edited git hooks remain behind ✗ Dismissed
Description
GitHookInstaller.Remove treats a missing command-and-event Entry() as proof that the entire hook
section is absent. If a user has removed those keys but left hook.kcap.enabled or another key in
the section, uninstall skips section removal and reports success.
Code

src/Capacitor.Cli/GitHookInstaller.cs[R39-40]

+        if (Git("--list") is null) return false;
+        return Entry() is null || Git("--remove-section", Section) is not null;
Relevance

●●● Strong

Accepted marker-cleanup precedent supports removing ownership sections even after users manually
prune recognized entries.

PR-#296

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Entry filters for command and event keys only; the new short-circuit returns success without running
--remove-section when neither key exists. Uninstall reports an error only when Remove returns
false.

src/Capacitor.Cli/GitHookInstaller.cs[35-40]
src/Capacitor.Cli/GitHookInstaller.cs[58-61]
src/Capacitor.Cli/Commands/UninstallCommand.cs[265-268]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Git-hook removal skips a partially populated kcap section because its presence check reads only command and event keys.
## Fix Focus Areas
- src/Capacitor.Cli/GitHookInstaller.cs[35-40]
- src/Capacitor.Cli/GitHookInstaller.cs[58-61]
## Recommended Fix
After confirming the config is readable, detect whether any key remains in the kcap hook section. Remove the section when present, and treat only a genuinely absent section as an idempotent success.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. An unreadable local registry now skips all agent cleanup ✓ Resolved
Description
HandleAsync now returns 1 at if (localFailed) return 1; before it stops recorders, removes any
integration or sweeps skills. Before this PR, an unreadable local registry or a failed
stacks.Create() only set hadFailures, and every other removal step still ran. So a corrupt
install.json now leaves every agent hook and MCP entry wired to kcap. The user is still told only
that "the local server is left as it is and the config directory is kept", which suggests the rest
of the uninstall went ahead.
Code

src/Capacitor.Cli/Commands/UninstallCommand.cs[90]

+        if (localFailed) return 1;
Relevance

●● Moderate

Cleanup sequencing concerns are credible, but no close precedent confirms cleanup should continue
after unreadable local state.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The early return comes right after the unreadable-registry message, which promises only that the
local server and config directory are kept. The README context line also says an unreadable registry
only keeps ~/.config/kcap/ and reports the uninstall incomplete. In the old code, `hadFailures =
localFailed` was set and cleanup carried on.

src/Capacitor.Cli/Commands/UninstallCommand.cs[73-93]
README.md[2681-2681]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`UninstallCommand.HandleAsync` now returns early when the local registry cannot be read, so no integrations are removed. The old behaviour (and the README) only kept the config directory and reported the uninstall as incomplete.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/UninstallCommand.cs[88-93]
- src/Capacitor.Cli/Commands/UninstallCommand.cs[177-219]

## Recommended Fix
Remove the early `return 1`. When `localFailed` is true, skip the local lifecycle path and call `CleanupAsync(null, hadFailures: true)`. That way integrations, hooks and skills are still removed, while the config directory and account registry are kept for a re-run. If returning early is intentional, change the message to say that nothing was changed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. A newly installed server can be removed ✓ Resolved
Description
The under-lease installation-change check is disabled by --keep-config, even though
LocalUninstall still removes the current server's service and runtime files in that mode. If
another setup completes after the initial inspection or confirmation but before lease acquisition,
uninstall proceeds against that new installation without presenting it for review.
Code

src/Capacitor.Cli/Commands/UninstallCommand.cs[R211-214]

+                if (!keepConfig && !purgeLocal && (state is LocalStackState.Installed or LocalStackState.Interrupted)
+                    && (!removeLocal || state is LocalStackState.Installed current && localState is LocalStackState.Installed previous
+                        && current.Installation.Id != previous.Installation.Id))
+                    throw new LocalLifecycleException("The local installation changed after confirmation. Run uninstall again to review its data deletion.");
Relevance

●● Moderate

The race is plausible, but keep-config intentionally avoids data-deletion consent; historical
evidence is inconclusive.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The initial state is read before the prompt and lease. The callback checks installation changes only
without --keep-config, while non-purge local uninstall removes the service and bin and run
directories.

src/Capacitor.Cli/Commands/UninstallCommand.cs[65-91]
src/Capacitor.Cli/Commands/UninstallCommand.cs[166-179]
src/Capacitor.Cli/Commands/UninstallCommand.cs[205-215]
src/Capacitor.Cli/LocalEdition/LocalUninstall.cs[24-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--keep-config` bypasses the post-confirmation installation-change check despite still removing a local server.
## Fix Focus Areas
- src/Capacitor.Cli/Commands/UninstallCommand.cs[205-215]
## Recommended Fix
Under the lease, reject a newly arrived or replaced installation that was not reviewed before confirmation, regardless of whether its data will be retained. Keep the interrupted-purge consent check before recovery.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. Uninstall never completes on machines without git ✓ Resolved
Description
GitHookInstaller.Remove returns false whenever a ~/.gitconfig exists and git config --list
returns null. RunGit returns null when git is missing, times out or exits non-zero (for example,
the macOS /usr/bin/git stub when the command-line tools are not installed). No kcap hook can exist
in that case, but uninstall still sets hadFailures. It then keeps ~/.config/kcap, the daemon
state and the account registry on every re-run, and reports the uninstall incomplete each time.
Code

src/Capacitor.Cli/GitHookInstaller.cs[R38-40]

+        if (!File.Exists(ConfigFile())) return true;
+        if (Git("--list") is null) return false;
+        return Entry() is null || Git("--remove-section", Section) is not null;
Relevance

●● Moderate

The failure behavior is intentional for retry safety, but treating absent Git as incomplete cleanup
may be overly broad.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
RunGit catches every exception (including the one thrown when the git executable is missing) and
returns null. It also returns null on a timeout or a non-zero exit. Remove treats that null from
--list as a failure, and CleanupAsync turns a false result into hadFailures = true, which
skips the config, daemon-directory and account-registry deletes.

src/Capacitor.Cli/GitHookInstaller.cs[35-41]
src/Capacitor.Cli/GitHookInstaller.cs[106-131]
src/Capacitor.Cli/Commands/UninstallCommand.cs[265-268]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GitHookInstaller.Remove` reports failure when git is not available, so uninstall stays incomplete forever on machines without a working git.

## Fix Focus Areas
- src/Capacitor.Cli/GitHookInstaller.cs[35-41]

## Recommended Fix
Before treating a null `--list` result as a failure, check whether git can run at all, for example with `InstalledGitVersion() is null`. If git is missing or unusable, return true: no hook can have been installed through it. Only return false when git runs but the config read or `--remove-section` fails.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

10. Cleanup errors are reported as local-server failures ✓ Resolved
Description
The outer catch around local.RunAsync also wraps all of CleanupAsync: integration removal,
config deletion and the account registry. Any IOException or UnauthorizedAccessException that
escapes those steps (for example from AppConfig.LoadProfileConfig) is printed as "Could not remove
the local server". The uninstall then returns 1 without running the remaining cleanup steps or
printing the usual incomplete-uninstall guidance, which sends users looking at the wrong component.
Code

src/Capacitor.Cli/Commands/UninstallCommand.cs[R216-218]

+        } catch (Exception ex) when (ex is LocalLifecycleException or LocalInstallFailure or LocalUpgradeFailure or IOException or UnauthorizedAccessException) {
+            await Console.Error.WriteLineAsync($"Could not remove the local server: {ex.Message}");
+            return 1;
Relevance

●●● Strong

Recent uninstall precedent favors accounting cleanup exceptions and continuing remaining cleanup
instead of aborting the whole operation.

PR-#1805

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
CleanupAsync is awaited inside the RunAsync lambda, so its exceptions reach the outer handler.
That handler only prints the local-server message.

src/Capacitor.Cli/Commands/UninstallCommand.cs[177-219]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Exceptions from `CleanupAsync` are caught by the local-server handler and reported with a misleading message.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/UninstallCommand.cs[177-219]

## Recommended Fix
Wrap the `CleanupAsync` call inside the lambda in its own try/catch that reports a generic uninstall error. Alternatively, track whether the local-removal phase finished and pick the message based on that.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 59 rules
✅ Cross-repo context — repo relationships
  Explored: repo: kurrent-io/kcap-server (sha: 8652ebb6) — View relationship
Review mode: Auto: 🧠 Deep: Bug-dense uninstall changes span lifecycle safety, integrations, filesystem cleanup, and recovery paths.

Grey Divider

Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread test/Capacitor.Cli.Tests.Unit/Commands/UninstallFilesTests.cs Outdated
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
Comment thread src/Capacitor.Cli/Commands/UninstallFiles.cs
Comment thread src/Capacitor.Cli/Commands/UninstallFiles.cs
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
Comment thread src/Capacitor.Cli/GitHookInstaller.cs
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
Comment thread src/Capacitor.Cli/GitHookInstaller.cs Outdated
Comment thread src/Capacitor.Cli/Commands/PluginCommand.cs
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs

@nortonandreev nortonandreev left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping lifecycle.lock and the retired-URL registry, and rechecking consent while the lock is held, both look right. Three behaviour changes need fixing before merge (inline): an uninstall that never had a local server now leaves ~/.config/kcap/local/, an unreadable registry now blocks every integration removal, and a missing git now makes uninstall incomplete forever. The rest are nits.

Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
Comment thread src/Capacitor.Cli/GitHookInstaller.cs
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
Comment thread src/Capacitor.App/Platform/Mac/MacUpdaterFiles.cs
Comment thread src/Capacitor.Cli/Commands/UninstallCommand.cs Outdated
…1843)

Taking the lifecycle lease creates the local root, so it is taken only once a setup has
created one: an uninstall must not leave behind the metadata it says it retains.
@nortonandreev

Copy link
Copy Markdown
Contributor Author

@CodeRabbit approve

@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved and changes approved.

@nortonandreev
nortonandreev merged commit a53a873 into main Oct 22, 2026
14 checks passed
@nortonandreev
nortonandreev deleted the norton/uninstall-followups branch October 22, 2026 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Complete uninstall cleanup and preserve local lifecycle safety

1 participant