feat(cli): installer subcommands, real config validation, status timeout

[malpern]

Summary

• Wires install, repair, uninstall, and inspect subcommands into the ArgumentParser CLI, delegating to InstallerEngine and PrivilegeBroker (addresses Codex P1 regression)
• Replaces config check file-presence check with real kanata --check validation via ConfigurationService.validateConfiguration()
• Adds --timeout flag to keypath status (default 30s) to prevent hangs from SMAppService synchronous IPC under launchd load
• Removes @MainActor from applyConfiguration(), isolating only the accesses that need it

Commands Added

Command	Description
keypath install [--json]	Install KeyPath services and components
keypath repair [--json]	Repair broken services (fast restart first, then full repair)
keypath uninstall [--delete-config] [--json]	Remove services and components
keypath inspect [--json]	Inspect system state without making changes

Commands Updated

Command	Change
keypath config check	Now runs kanata --check instead of file presence check; adds --json
keypath status	New --timeout <seconds> flag (default 30)

Test plan

• swift build --product keypath compiles cleanly
• swift build --product KeyPath compiles cleanly
• swift test — all 307 tests pass, no regressions
• swiftformat applied

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Code Review - PR 223

Overall this is a solid, well-scoped PR. All three headline improvements are correct architectural moves. A few issues worth addressing before merge:

---

Bug - stdout pollution before JSON output (Medium)

In InstallCommand.swift the preamble messages (e.g. 'Starting installation...') go to stdout even when --json is passed. Same issue in Repair and Uninstall. Scripts piping output to jq will see garbled content mixing the preamble string with the JSON object.

Fix: route preamble through printErr() so it goes to stderr, or gate it with 'if !json'.

---

Bug - Force unwrap in Timeout.swift (Low)

let result = try await group.next()!

The two-task setup makes nil practically impossible today, but force-unwraps are fragile against future refactors. Prefer:

guard let result = try await group.next() else { throw TimeoutError() }

---

Issue - UInt64 overflow in sleep duration

try await Task.sleep(nanoseconds: UInt64(seconds) * 1_000_000_000)

A very large --timeout value overflows. Consider capping the input or using the Duration-based API on macOS 13+:

try await Task.sleep(for: .seconds(seconds))

---

Design - planStatus as raw String in CLIInspectResult

planStatus is 'ready' or 'blocked' -- stringly-typed for JSON consumers. A Codable enum would be more robust; at minimum add a comment near the struct documenting the valid values.

---

Quality - printInstallerReport is a module-level free function

No access modifier makes it internal across all of KeyPathCLI. Mark it private or scope it inside KeyPathTool to prevent future naming conflicts.

---

Minor - Two InstallerEngine instances in runRepair()

A first engine is created for inspection, then a second InstallerEngine() is created for the full repair path. Reuse the first instance by hoisting it before the branch.

---

Tests - no new coverage for the 4 new commands

All 307 existing tests pass, which is great. But the fast-repair path decision logic and inspect-plan serialisation have no test coverage. Not a merge blocker, but worth a follow-up issue.

---

Positives

• Removing @mainactor from applyConfiguration() and isolating it to only the two accesses that actually need it is exactly right per the architectural guidelines (avoiding repeated synchronous IPC on a hot path).
• withThrowingTimeout using withThrowingTaskGroup correctly cancels the losing task on completion.
• validateConfig() replacing the file-presence stub with real kanata --check is a meaningful correctness improvement.
• Consistent --json output across all new commands is a good scripting affordance.
• The SwiftFormat fix in RemapCommand.swift (using comma instead of && in the if condition) is correct.

[claude]

Code Review

Overall this is solid work. The CLI subcommands, real config validation, and status timeout address real pain points. The UI refactoring (Shift slot, multi-tap dropdown, glow effects) is coherent. A few things worth fixing before merge.

BUGS

1. Force-unwrap in withThrowingTimeout (Timeout.swift line 15)

group.next() can return nil if all tasks complete without producing a value. This crashes. Fix: guard let result = try await group.next() else { throw TimeoutError() }

2. Progress messages corrupt JSON output (InstallCommand.swift)

In Install, Repair, and Uninstall, progress strings land on stdout before the JSON output runs, breaking automated JSON consumers. Fix: suppress in JSON mode or redirect via printErr().

ARCHITECTURE AND CLAUDE.md VIOLATIONS

3. runRepair fast-path calls inspectSystem() which does slow SMAppService IPC

After coordinator.restartService() succeeds, the code calls engine.inspectSystem() to verify health. But inspectSystem() calls SMAppService.status, which CLAUDE.md documents as synchronous IPC taking 10-30s under launchd load -- the exact problem that motivated the new --timeout flag on keypath status. The fast-repair path can therefore be just as slow. Consider verifying liveness via the TCP health check only (Kanata responding on TCP = service is live), or document that this path has the same latency caveat.

4. runRepair creates two InstallerEngine instances

One for fast-path inspection, one for full repair. If system state changes between calls, the second engine operates on stale context. Reuse the same instance, or document the intentional race window.

5. PhysicalLayout.hasDrawerButtons uses an unexplained magic constant

The sentinel 0xFFFF in keys.contains { keyCode == 0xFFFF } has no comment or named constant. Future maintainers will have no idea what it represents.

MINOR ISSUES

6. inputConditionDisplayText missing the 4x tap case (OverlayMapperSection.swift)

The switch only handles case 2 and case 3; default: nil means a 4x tap condition shows Everywhere instead of 4x Tap. Since tapCounts = [2, 3, 4], add case 4.

7. Duplicate JSON encoder setup across 4 CLI commands

The same 3-line JSONEncoder setup (prettyPrinted + sortedKeys) is copy-pasted into all 4 new commands and Config.Check. Extract to a shared static helper.

8. @mainactor on validateConfig() inconsistent with this PRs refactoring direction

applyConfiguration() had @mainactor removed in this PR (correct). validateConfig() was added with it. If neither ConfigurationService.current() nor validateConfiguration() require the main actor, the annotation adds unnecessary actor hops in a CLI context.

9. Non-public initializers on public CLIInstallerReport

CLIInstallerReport is public but both initializers are internal. Either make them public or demote the struct to internal.

WHAT IS WORKING WELL

• showTransientStatus cancellation is correct: try? await Task.sleep plus guard !Task.isCancelled is the right pattern.

• Removing KanataConfigGenerator from MapperViewModel and routing everything through saveCustomRule -> regenerateConfigFromCollections closes the dual-path inconsistency where complex mappings used a different code path than simple ones. Good simplification.

• isShiftedOutputDefault preventing the system-default shift from being saved as a custom rule in currentShiftedOutputKanataString() is clean and correct.

• BehaviorSlot.shift as a first-class slot rather than a sub-mode of .tap is the right abstraction.

• Timeout utility is simple and correct (modulo the force-unwrap above).

[claude]

PR Review test placeholder

[claude]

PR Review: feat(cli): installer subcommands, real config validation, status timeout

Overall this is a solid, well-motivated PR. The migration from KeyPathCLI/KeyPathCLIEntrypoint to proper ArgumentParser subcommands is the right architectural move. The @mainactor narrowing in applyConfiguration() and the --timeout guard on status are both targeted, correct fixes.

---

Issues worth addressing before merge

1. Negative/zero --timeout will trap at runtime

Timeout.swift:12 does UInt64(seconds) * 1_000_000_000. UInt64(-1) traps at runtime; UInt64(0) races the work task. No validate() on StatusCommand rejects non-positive values. Suggest adding a validate() method that throws ValidationError when timeout < 1.

2. Test coverage regression: 144 lines deleted, 0 added for new commands

KeyPathCLITests.swift is removed because KeyPathCLI is gone -- fair. But Install, Repair, Uninstall, Inspect, and the new validateConfig() path have no unit tests. The fast-repair path in runRepair() is particularly tricky (two engine instances, conditional early return) and would benefit from CLIFacadeTests. All 307 tests pass covers the old surface area, not the new one.

---

Notable design feedback

3. runRepair() fast path calls inspectSystem() -- the slow IPC call you are trying to avoid

The motivation for --timeout on status is that SMAppService.status can block 10-30s under load (per CLAUDE.md). The fast-repair path in CLIFacade.runRepair() calls engine.inspectSystem() unconditionally after restartService() returns true -- same latency exposure. Consider wrapping it with the timeout helper or trusting the restartService() bool and skipping the post-check.

4. runRepair() creates two InstallerEngine() instances

Fast path creates one engine for inspection; if it falls through, a second is created for full repair. A single let engine = InstallerEngine() before the branch cleans this up.

5. printInstallerReport is a free function at module scope

InstallCommand.swift line 131 is globally visible and will conflict with anything else in the module defining printInstallerReport. Should be private func or a static func on a private helper type.

6. CLIInspectResult omits timestamp

CLIStatusResult has a timestamp: Date field. CLIInspectResult does not, even though engine.inspectSystem() returns a SystemContext with a timestamp. Scripts correlating inspect output with logs will have no time anchor.

7. withThrowingTimeout force-unwrap

group.next()! is safe given the structure (two tasks always added), but non-obvious. A guard-let with explicit throw is clearer.

---

Things done well

• Removing CLI dispatch from Main.swift and KeyPathCLIEntrypoint is the right call -- the app binary should not embed a CLI entry point.
• config check now invokes kanata --check instead of a file-presence test -- meaningful improvement.
• @mainactor narrowing in applyConfiguration() via MainActor.run is correct and idiomatic.
• showTransientStatus() with task cancellation is a clean pattern; the previous persistent-until-next-save status was a UX paper cut.
• isShiftedOutputDefault tracking correctly threads through recording, loading, and currentShiftedOutputKanataString() -- prevents phantom fork rules for system-default shift pairs.
• enrichWithCustomShiftLabels is pure (mapping + rules as parameters), keeping it testable and side-effect-free.
• Accessibility identifiers on Karabiner import sheet items are a useful addition for UI automation testing.
• Removing the duplicate publish-help-to-web.sh registry entry for neovim-terminal is a good cleanup.

---

Minor nits

• PhysicalLayout.hasDrawerButtons uses magic constant 0xFFFF -- a named constant would make intent clearer.
• After removing KanataConfigGenerator from the global mapping save path, errors in the reload pipeline are absorbed as saveCustomRule returning false. Worth a comment.
• The condition separator fix in RemapCommand.swift (changing && to , in the if-let syntax) is a correct style improvement.