feat(uninstall): opt-in removal of the VirtualHID driver (#679)

[malpern]

Closes #679.

The user-facing uninstall already tears down the daemon, helper, and app, but left the Karabiner VirtualHID driver installed — and InstallerEngine.uninstall's _ = broker // Reserved for future privileged uninstall steps was an unused hook.

Change

• New opt-in "Also remove the virtual keyboard driver" checkbox in the uninstall dialog (default off — VHID is shared; other tools like Karabiner-Elements may rely on it).
• Flag threads UninstallKeyPathDialog → KanataViewModel → RuntimeCoordinator → InstallerEngine.uninstall, which now uses the broker to call uninstallVirtualHIDDrivers() before the main teardown (that self-destructs the privileged helper this step depends on).
• Best-effort: a VHID failure logs and continues, so it never blocks removing KeyPath itself.
• Resolves the "dead stub" concern from Uninstall is a placeholder — no privileged teardown #679 by implementing the reserved broker hook (per maintainer decision: opt-in toggle).

⚠️ Verification

The uninstall path is destructive and can't be exercised in automated tests. Before merging, please verify a real uninstall manually:

• toggle on → VirtualHID driver/daemon removed
• toggle off → driver preserved (current behavior)

Not auto-merging for this reason.

[claude]

Code Review — feat(uninstall): opt-in removal of the VirtualHID driver

Overview

Clean, minimal implementation. The flag threads cleanly through all four layers (UninstallKeyPathDialog → KanataViewModel → RuntimeCoordinator → InstallerEngine) with consistent = false defaults at every hop. Ordering constraint (VHID before privileged-helper teardown) is correct and well-commented. Default-off is the right call for a shared driver.

---

Issues & Suggestions

1. Silent VHID failure not surfaced in the returned InstallerReport (medium)

When removeVirtualHID: true and broker.uninstallVirtualHIDDrivers() throws, the error is logged to AppLogger and execution continues — but the InstallerReport returned by uninstall() has no trace of it. The InstallerReport has a logs: [String] field specifically for this use case. The user sees "Uninstall Complete" even if the driver is still installed.

Suggested fix — add the failure to the logs array that gets passed into the report:

var extraLogs: [String] = []

if removeVirtualHID {
    do {
        try await broker.uninstallVirtualHIDDrivers()
        AppLogger.shared.log("🗑️ [InstallerEngine] Virtual HID driver removed")
    } catch {
        let msg = "Virtual HID removal failed (driver may still be installed): \(error)"
        AppLogger.shared.log("⚠️ [InstallerEngine] \(msg)")
        extraLogs.append(msg)
    }
}

Then pass extraLogs into the final InstallerReport. This way both the UI (lastError or a warning label) and any programmatic callers can know the driver was not removed.

2. UI shows "Uninstall Complete" even when VHID removal silently failed (medium)

Following from #1: performUninstall() in UninstallKeyPathDialog sets didSucceed = report.success and lastError = report.failureReason. If VHID fails but the main uninstall succeeds, report.success == true and report.failureReason == nil, so the success state is displayed with no indication the driver is still present. This could leave the user confused when Karabiner-Elements still works (or when the driver is still listed).

Consider displaying a soft warning (e.g., an inline .caption-level note) when the report logs contain a VHID failure message.

3. Unnecessary comment on line 40 of the dialog (minor)

// Optional: also remove the shared Karabiner virtual keyboard driver.

Per CLAUDE.md: "Only add [a comment] when the WHY is non-obvious." The toggle label already says what this does. The comment can be removed.

4. deleteConfig: false hardcoded on line 108 of the dialog (nit)

Not introduced by this PR, but worth noting: the dialog always passes deleteConfig: false. If that's intentional (config is always preserved in this flow), a brief doc comment on performUninstall() or at the call site would make the invariant explicit and prevent a future contributor from assuming it's a placeholder.

---

What's Good

• Correct ordering: VHID removal before createUninstallCoordinator runs — the critical constraint is clearly documented in a comment.
• Best-effort semantics are appropriate; a VHID failure should never block removal of KeyPath itself.
• accessibilityIdentifier("uninstall-remove-vhid-toggle") present for UI testing.
• removeVirtualHID flag disabled while isRunning — prevents mid-flight state change.
• Default = false throughout the call chain — safe for existing callers.

---

Verification

Author correctly flags this as requiring manual verification. Before merging, confirm:

• Toggle on: driver/daemon absent after uninstall (check kextstat or systemextensionsctl list).
• Toggle off: driver preserved.
• Toggle on + VHID removal failure (e.g., helper not running): uninstall still completes, failure is surfaced somewhere visible.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93690c7c20

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Route opted-in VHID removal through fallback path

When the privileged helper is missing or not responding, broker.uninstallVirtualHIDDrivers() fails in release because the router's privileged-helper branch only calls helperManager.uninstallVirtualHIDDrivers() (no sudo/script fallback), but the catch here swallows that failure and then the normal uninstall can still succeed via UninstallCoordinator's admin-prompt script fallback. In that scenario, a user who checked “Also remove the virtual keyboard driver” gets a successful uninstall while the VirtualHID driver is preserved, so this path needs either a fallback before continuing or a report that the opted-in removal did not happen.

Useful? React with 👍 / 👎.