Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfea143956
ℹ️ 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".
| throw NSError(domain: "KeyPathOutputBridge", code: Int(code), userInfo: [NSLocalizedDescriptionKey: "failed to bind output bridge socket: \(code)"]) | ||
| } | ||
|
|
||
| chmod(session.socketPath, 0o666) |
There was a problem hiding this comment.
The companion opens each UNIX socket as world-writable (0o666), and request handling accepts .emitKey directly without checking peer credentials or requiring a prior authenticated handshake (handle(clientFD:) routes .emitKey immediately). Because the helper also creates the socket/session directories with 755 permissions (HelperService.ensureKanataOutputBridgeDirectory), any local user can discover/connect to the socket and inject privileged key events, which is a security regression.
Useful? React with 👍 / 👎.
| reply(true, nil) | ||
| NSLog("[KeyPathHelper] restartKanataOutputBridgeCompanion acknowledged, scheduling async restart") | ||
|
|
||
| DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + 0.5) { |
There was a problem hiding this comment.
This method acknowledges success before attempting the restart, then performs the actual restart asynchronously. Callers such as KanataSplitRuntimeHostService.restartCompanionAndRecoverPersistentHost() immediately query companion health and treat a negative result as a hard recovery failure, so this race can incorrectly fail recovery and tear down the split runtime even when the delayed restart would have succeeded.
Useful? React with 👍 / 👎.
|
PR Review pending - see detailed review below |
|
Bug 1 - Duplicate isKarabinerElementsRunning() in DiagnosticsService.getSystemDiagnostics(): Lines 382 and 427 both await this method, spawning two separate pgrep calls and appending two distinct diagnostics (warning + error) for the same condition. Capture the result once at the top and reuse it. |
|
Bug 2 - Hard-coded TCP port 37001 in DiagnosticsService: fetchTcpStatusInfo() and fetchTcpHello() both construct KanataTCPClient(port: 37001) directly. Every other caller uses PreferencesService.shared.tcpServerPort. If the user changes the port in preferences, these diagnostic methods silently connect to the wrong port. |
|
Bug 3 - currentRuntimeStatus() underreports when legacy daemon is active: RuntimeCoordinator+ServiceManagement.swift:180-186 returns .stopped whenever the split runtime host is not running, even if the legacy RecoveryDaemonService is still active. resetToDefaultConfig() in RuntimeCoordinator.swift:877 checks runtimeStatus.isRunning before triggering a TCP reload. In a half-migrated state this causes the reset to silently skip the reload. Even if the legacy daemon is out of scope going forward, the silent skip should at minimum emit a log warning. |
|
Dead code - fetchTcpStatusInfo() has no callers: DiagnosticsService.swift:886 defines private func fetchTcpStatusInfo() with no call sites anywhere in the codebase. Should be removed. Hot-path logging noise - buildUIState() emits .log() on every state update: RuntimeCoordinator.swift:60 logs customRules.count at INFO level. buildUIState() is called on every SwiftUI redraw, save, and layer change. This floods keypath-debug.log under normal usage. Should be demoted to .debug() or removed. |
|
Consistency - prepareExperimentalOutputBridgeEnvironment creates a fresh KanataOutputBridgeCompanionManager each call: KanataRuntimePathCoordinator.swift:73-76 allocates KanataOutputBridgeCompanionManager(helperManager:) per invocation instead of using .shared. Every other call site in this PR uses .shared. Likely an oversight. Consistency - activateKanataOutputBridgeSession 45-second timeout undocumented: HelperManager+RequestHandlers.swift:315 uses 45s vs. the 30s default for all other calls. A brief comment explaining why this operation needs more time would help future readers. Stale comments - RuntimeCoordinator.swift doc block line counts are wrong: Header says +Lifecycle.swift is ~77 lines (actual: 202) and +ServiceManagement.swift is ~119 lines (actual: 199). Typo at RuntimeCoordinator.swift:311: Initialize EngineClien (missing t). |
|
Summary - What is solid: Removal of KanataService.swift (571 lines), ProcessCoordinator.swift (124 lines), and ProcessManager.swift (257 lines) cleans up substantial legacy complexity without apparent regressions. HelperXPCCallCompletionState lock-based completion guard correctly prevents double-resume of CheckedContinuation under timeout plus XPC races. restartKanataOutputBridgeCompanion clearConnection() retry pattern is good defensive hygiene. DEBUG test seams (testPersistentHostPID, testStartPersistentError) follow the established KeyPathTestCase pattern correctly. Validation order and PermissionOracle flow are unchanged and intact. The split-runtime-first hard break in startKanata() is a clean, intentional design decision that the code executes correctly. |
PR Review: Finalize Split Runtime CutoverThis is a substantial architectural change that commits KeyPath to the split-runtime path as the only supported runtime. The code is well-structured overall and the separation of concerns is improved. Here are my observations: Issues Worth Addressing1. No fallback if split runtime fails — user is left without remapping
2. Race condition in guard activeHostProcess?.isRunning != true else {
return activeHostProcess?.processIdentifier ?? 0 // returns 0 if process exited between guard and this line
}PID 0 is 3. func currentRuntimeStatus() async -> RuntimeStatus {
if KanataSplitRuntimeHostService.shared.isPersistentPassthruHostRunning {
return .running(pid: ...)
}
return .stopped // doesn't account for legacy daemon still running during cutover window
}During the legacy-to-split cutover in 4.
5.
More importantly: this check only runs after the full 20-second readiness timeout. If an existing Kanata process holds the port, the user waits 20 seconds before seeing the Design Observations 6. The new check reads Kanata's stderr log and looks for 7. Duplicated environment setup in Both functions build nearly identical 8. Companion monitor catch block swallows the thrown error In 9. Rust The function probes TCP port availability by binding and immediately dropping a listener, then Positive Changes
Minor
|
|
test |
|
Review Part 1 of 3 - Bugs and Architecture findings for PR 225 Finalize Split Runtime Cutover. More details in comments 2 and 3. |
|
BUGS AND CORRECTNESS: (1) waitUntilExit blocks MainActor in launchPassthruHost -- KanataSplitRuntimeHostService.launchPassthruHost is MainActor and calls process.waitUntilExit() line 155, a synchronous blocking call that freezes the actor for up to 8s. Fix with a detached wait. (2) currentRuntimeStatus does not check legacy recovery daemon -- returns .stopped even if legacy daemon is active, causing callers like resetToDefaultConfig to skip TCP reload during daemon transition. Check recoveryDaemonService.isRecoveryDaemonRunning() as secondary condition. (3) Variable shadowing in startKanata and restartKanata -- the function parameter reason is shadowed by the enum case binding. Produces a Swift warning. Rename binding to evalReason. |
|
ARCHITECTURE AND PERFORMANCE: (4) Multiple evaluateCurrentPath() calls in startup hot path -- violates CLAUDE.md anti-pattern. Called at least 3x: in engine.inspectSystem(), via shouldUseSplitRuntimeHost(), and inside startKanata() via currentSplitRuntimeDecision(). Each involves XPC to the helper. Cache at top of performInitialization and pass down. (5) Env var key constants duplicated -- KEYPATH_EXPERIMENTAL_OUTPUT_BRIDGE_SESSION and KEYPATH_EXPERIMENTAL_OUTPUT_BRIDGE_SOCKET defined as private in KanataSplitRuntimeHostService lines 36-37 AND as public statics in KanataRuntimePathCoordinator lines 6-7. Remove private copies, reference coordinator constants. (6) Duplicate Karabiner conflict diagnostic -- getSystemDiagnostics emits .warning at line 384 AND .error at line 428 for the same karabinerElementsRunning condition. Merge into one. (7) isOneShotProbeMode logic duplicated in RuntimeCoordinator line 159 and KeyPathApp line 27 with same 4 env-var checks. Should share a single source of truth. |
|
RUST CODE: (8) aarch64-only build in build-kanata-host-bridge.sh -- hardcodes --target aarch64-apple-darwin, intentional given macOS 15 min but should be documented in script header. (9) Unnecessary .clone() at lib.rs:139 -- tcp_server_address: tcp_server_address.clone() may be moveable into ValidatedArgs instead. (10) Test bypasses public API -- lib.rs:726 reads output_rx directly via unsafe pointer cast instead of keypath_kanata_bridge_passthru_try_recv_output, fragile to field reordering. TESTING GAPS: (11) No unit tests for new Swift services -- KanataSplitRuntimeHostService, KanataOutputBridgeCompanionManager, KanataOutputBridgeSmokeService have zero Swift test coverage. Test seams testPersistentHostPID and testStartPersistentError are wired but not exercised in any test. (12) analyzeLogFile loads entire log file into memory before tailing -- String(contentsOfFile:) then .suffix(100) is wasteful for large logs. Use file-handle seek-to-end instead. |
|
WHAT IS GOOD: Removing 5000+ lines of legacy code (ProcessManager, KanataService, HelperBackedPrivilegedOperations) is a meaningful net reduction in surface area. The test seams in KanataSplitRuntimeHostService follow the established project pattern. The Rust C-ABI bridge is cleanly structured with consistent null-pointer guards and a clear error-buffer convention. Hard-failing in startKanata when the split runtime is not viable (no silent legacy fallback) is exactly right for committing to the new architecture. stopPersistentPassthruHost correctly sets expectedPersistentHostTermination = true before terminate(), preventing false unexpected-exit alerts. The companion health monitor loop with the isRecoveringSplitRuntimeCompanion re-entrancy guard prevents a recovery storm if the companion is persistently unhealthy. SUMMARY: Items 1 and 3 are correctness issues to address before merging. Items 4 and 5 are architectural concerns that CLAUDE.md explicitly identifies as anti-patterns. Item 11 (test coverage gap) is the most significant long-term risk. Generated with Claude Code. |
Code Review — Finalize Split Runtime CutoverOverall this is a well-structured cutover. The removal of 🟡 Medium — Potential double-resume in
|
Code Review — PR #225: Finalize split runtime cutoverThis is a substantial, well-structured PR. The split-runtime architecture cleanly separates input capture (in-process via the bundled Kanata host) from output injection (via the privileged helper's output bridge companion). The existing patterns and anti-patterns from CLAUDE.md are followed throughout. Below are my findings, from most to least significant. 🐛 Bugs / Correctness Issues1. In if self.activeHostProcess?.processIdentifier == process.processIdentifier {
self.activeHostProcess = nil
self.expectedPersistentHostTermination = false // ← Never reached
}Since Suggestion: Either set 2. stopPersistentPassthruHost()
try await Task.sleep(for: .milliseconds(250))
return try await startPersistentPassthruHost(...)
Suggestion: Use a
|
| Priority | Issue |
|---|---|
| 🐛 Medium | stopPersistentPassthruHost termination handler cleanup is dead code; expectedPersistentHostTermination not reliably reset |
| 🐛 Medium | 250ms sleep in companion restart doesn't wait for actual process exit |
Debug system actions (stop-host-passthru, etc.) reachable in production via keypath:// URI |
|
Magic string "Split Runtime Host" in init cutover logic |
|
| Duplicate Karabiner conflict diagnostics (warning + error for same condition) | |
| ℹ️ Info | 45s XPC timeout has no user-visible feedback on main actor |
| ℹ️ Info | /var/tmp session env file is world-readable |
Overall the architecture is solid and the cutover approach is sound. The two correctness bugs are worth fixing before merging; the rest are polish items.
PR Review: Finalize Split Runtime Cutover (Follow-up)Existing reviews have covered the main issues well. This review focuses on a few additional findings. Bug: NotificationCenter observers registered without storing tokens
// Token discarded — cannot be removed later
NotificationCenter.default.addObserver(forName: .configAffectingPreferenceChanged, ...) { ... }
NotificationCenter.default.addObserver(forName: .splitRuntimeHostExited, ...) { ... }The
Fix: store the tokens and remove them in private var notificationTokens: [NSObjectProtocol] = []
// In init:
notificationTokens.append(NotificationCenter.default.addObserver(...) { ... })
// In deinit:
notificationTokens.forEach { NotificationCenter.default.removeObserver($0) }
splitRuntimeCompanionMonitorTask?.cancel()Minor:
|
|
test |
Final Consolidated Review — PR 225: Finalize Split Runtime CutoverPrior reviews in this thread have been thorough. This comment consolidates the key findings into a single clear merge-readiness verdict. Must-Fix Before Merge1. stopPersistentPassthruHost termination handler is dead code The handler guards on Fix: reset 2. 250ms sleep in companion restart does not guarantee old process has exited
Fix: wait for 3. Debug system actions reachable in production via keypath:// URI
Fix: wrap these cases in 4. NotificationCenter block-observer tokens discarded in RuntimeCoordinator.init Two Fix: store the tokens and remove them in deinit. Recommended to Address
What Is Well Done
SummaryThe four must-fix items are correctness and security issues that could surface during normal usage. The architecture is sound and the split-runtime-first cutover is a clean, intentional commitment to the new path. Fix those four and this is ready to merge. Generated with Claude Code |
PR Review: Finalize Split Runtime CutoverOverall this is a well-structured architectural change. The split runtime approach is cleaner than the legacy LaunchDaemon path, and removing 🔴 Critical / Bugs1. Double
// ServiceHealthChecker.swift ~line 305
let kanataState = await KanataDaemonManager.shared.refreshManagementState() // IPC call 1
...
if kanataState.isSMAppServiceManaged {
let runtimeSnapshot = await checkKanataServiceRuntimeSnapshot() // triggers call 2!
🟠 High Severity2. TOCTOU race in TCP port binding (Rust bridge, The code does a preflight This isn't just theoretical — Kanata itself can race for the same port. The preflight bind should either be eliminated (accept the better error from 3. XPC session activation timeout is 45 seconds // HelperManager+RequestHandlers.swift:317
try await executeXPCCall("activateKanataOutputBridgeSession", timeout: 45.0)45 seconds is a very long UI-blocking timeout. If this call hangs on a healthy system it will appear frozen for nearly a minute. Consider a shorter timeout (10-15s) with a more actionable error message, reserving the longer timeout for CI via an environment variable override. 4. // RuntimeCoordinator+ServiceManagement.swift:155-159
let splitDecision = await currentSplitRuntimeDecision() // async eval
if KanataSplitRuntimeHostService.shared.isPersistentPassthruHostRunning {
let stopped = await stopKanata(...)
guard stopped else { return false }
return await startKanata(...) // splitDecision unused in this branch
}In the fast path (host already running), 🟡 Medium Severity5.
6. // KanataDaemonManager.swift:708
try await Task.sleep(for: .seconds(2))Fixed sleeps are fragile — too short on slow machines, too long otherwise. Replace with a poll loop (e.g., polling 7. Error message quality in message += ". Toggle the service again to restart the split runtime host.""Toggle the service" is cryptic to users who don't know what that means. Suggest: "Use the Start button in KeyPath to restart." 8. Intel Mac exclusion undocumented
🟢 Low Severity / Style9. Rust bridge: unnecessary clone in // lib.rs:139
tcp_server_address: tcp_server_address.clone(),
10. The new 11. reason.contains("Approval required")String matching on error messages is fragile — if the message ever changes (localization, wording update), this silently stops working. Consider a dedicated ✅ What's Working Well
Reviewed by Claude Sonnet 4.6 |
PR #225 Code Review: Finalize Split Runtime Cutover\n\nThis is a large PR. |
|
PR 225 Code Review: Finalize Split Runtime Cutover\n\nLarge PR (317 files). Several blocking issues found.\n\nCRITICAL Issues:\n1. Restart race in restartKanata() - stopPersistentPassthruHost() returns before process terminates; use waitForPersistentHostExit() pattern.\n2. XPC on MainActor every 5s - startSplitRuntimeCompanionMonitor() calls getKanataOutputBridgeStatus() (30s XPC) on main actor.\n3. evaluateCurrentPath() called multiple times per operation - uncached, duplicated in inspectSystem.\n4. checkKanataInputCaptureStatus() reads legacy daemon log path, not split runtime host path.\n5. TOCTOU in PrivilegedCommandRunner.swift - file written then chmod'd separately.\n\nHigh Severity:\n6. assertionFailure removed from concurrent XPC detection.\n7. RecoveryDaemonService calls SMAppService.status on every start/stop (hot path).\n8. Bridge session files in world-readable /var/tmp.\n9. bootstrapOutputBridgeCompanion blocks XPC thread with up to 2s usleep.\n\nMedium:\n10. currentRuntimeStatus() ignores recovery daemon state.\n11. KanataRuntimeHost.current() uses Bundle.main.bundlePath - breaks in tests.\n12. Foundation.FileManager() used instead of FileManager.default.\n13. isOneShotProbeEnvironment missing hostPassthruCaptureEnvKey.\n14. restartKanata logic bifurcation.\n15. Silent pid == 0 guard in termination handler.\n16. Double launchctl calls per health check.\n\nMust fix before merge: Issues 1-5. Note: ADR-032 is Proposed but PR ships split runtime as production -- update ADR status.\n\nReview generated by Claude Code |
PR #225 Code Review: Finalize Split Runtime CutoverThis is a large (317 files, +14,701/-5,386 lines), architecturally significant PR. The overall direction is sound, but there are several issues ranging from blocking race conditions to performance hot paths that should be addressed. CRITICAL / Blocking Issues1. Restart Race Condition in restartKanata()Sources/KeyPathAppKit/Services/KanataSplitRuntimeHostService.swift stopPersistentPassthruHost() calls process.terminate() and returns immediately. restartKanata() then calls startKanata() which calls startPersistentPassthruHost(). But activeHostProcess is still set (cleared asynchronously in the termination handler), so the guard restartPersistentPassthruHostAfterCompanionRestart() correctly calls waitForPersistentHostExit() -- restartKanata() should use the same pattern. 2. XPC on Main Actor Every 5 SecondsSources/KeyPathAppKit/Managers/RuntimeCoordinator+Lifecycle.swift startSplitRuntimeCompanionMonitor() runs a 3. evaluateCurrentPath() Called Multiple Times Per OperationSources/KeyPathAppKit/Services/KanataRuntimePathCoordinator.swift evaluateCurrentPath() makes two XPC calls (testHelperFunctionality + getKanataOutputBridgeStatus) and is called from startKanata(), stopKanata(), and restartKanata(). Additionally, InstallerEngine.inspectSystem() calls evaluateCurrentPath() then calls getKanataOutputBridgeStatus() again immediately -- two XPC calls where one suffices. Cache results within a single validation cycle. 4. checkKanataInputCaptureStatus() Reads from Wrong Log PathServiceHealthChecker parses Kanata stderr for iohiddeviceopen error from KeyPathConstants.Logs.kanataStderr (the legacy daemon path). The split runtime host writes to NSTemporaryDirectory() + keypath-host-passthru-live-stderr.log. These are different paths -- this function will never detect IOHIDDeviceOpen errors from the split runtime host. 5. TOCTOU Risk in Temp Script File CreationSources/KeyPathCore/PrivilegedCommandRunner.swift -- file is written at 0o600, then chmod'd in a separate syscall. Any process can open the file between write and chmod, and this file is executed with admin privileges via osascript. Write with correct permissions atomically or use a process-private directory. High Severity Issues6. assertionFailure Removed from Concurrent XPC DetectionSources/KeyPathAppKit/Core/HelperManager+RequestHandlers.swift -- the concurrent XPC call guard previously fired assertionFailure in debug builds. This PR replaces it with a log. Concurrent XPC calls now silently proceed in debug builds, potentially hiding regressions. 7. RecoveryDaemonService.evaluateStatus() Calls SMAppService.status on Every Start/StopTwo Task.detached blocks call SMAppService.daemon(plistName:).status synchronously -- triggered on every isRecoveryDaemonRunning() call from startKanata() and stopKanata(). Per CLAUDE.md this is prohibited in hot paths. 8. Bridge Session Files Written to World-Readable Path/var/tmp/keypath-host-passthru-bridge-env.txt contains session IDs and socket paths for a privileged bridge. /var/tmp is world-readable. Consider /var/run or a root-owned path. 9. HelperService.bootstrapOutputBridgeCompanion Blocks XPC Thread with usleepUp to 2 full seconds of blocking sleep (200ms + 400ms + 600ms + 800ms) on the helper XPC service thread across 5 retry attempts. Reduce retry count or sleep duration. Medium Severity / Code Quality10. currentRuntimeStatus() Does Not Reflect Recovery Daemon StateOn mid-migration systems where the legacy recovery daemon is still running, currentRuntimeStatus() always reports .stopped. The RecoveryDaemonService state is never reflected. 11. KanataRuntimeHost.current() Uses Bundle.main.bundlePath -- Breaks in TestsIn unit tests, Bundle.main.bundlePath points to the XCTest runner. Any test calling KanataRuntimeHost.current() directly (not through the testDecision override) gets wrong binary paths. No test-seam override exists for this method. 12. Foundation.FileManager() vs FileManager.defaultMany files now use Foundation.FileManager() (new instance per call). FileManager.default is the thread-safe singleton. Use Foundation.FileManager.default where namespace disambiguation is needed, not Foundation.FileManager(). 13. isOneShotProbeEnvironment Missing hostPassthruCaptureEnvKeyAppDelegate.isOneShotProbeEnvironment checks 4 env keys but omits hostPassthruCaptureEnvKey. Running with KEYPATH_ENABLE_HOST_PASSTHRU_CAPTURE=1 will not enter probe mode. 14. restartKanata Logic BifurcationChecks isPersistentPassthruHostRunning before the splitDecision switch, creating two independent code paths that can disagree when the companion crashes mid-restart. 15. Silent guard pid > 0 in Termination HandlerhandleSplitRuntimeHostExit(pid:) silently returns with no log if pid == 0, making early-exit races invisible in logs. 16. Double launchctl Calls per Health CheckKanataDaemonManager.preferredLaunchctlTargets now returns two targets, doubling subprocess overhead on every health check. Consider only falling back to the second target on failure of the first. Test Coverage GapsGood additions: RuntimeCoordinatorTests, KanataRuntimeHostTests, KanataRuntimePathDecisionTests, KanataOutputBridgeSmokeServiceTests. Gaps: checkSplitRuntimeCompanionHealth() polling loop has no tests for companion-down, recovery success, or recovery failure. evaluateKanataLaunchctlRunningState fallback path (first target fails, second succeeds) is untested. KanataRuntimePathEvaluator.decide() with requiresPrivilegedBridge: false is untested. RecoveryDaemonServiceIntegrationTests.testStatusRefresh_ShouldDetectChanges asserts XCTAssertNotEqual(status, .unknown) -- too weak; passes even if status is .failed. CLAUDE.md Compliance
RecommendationsMust fix before merge:
Should fix:
Note: ADR-032 is marked Proposed but this PR ships split runtime as production. Consider updating ADR status to Accepted before merge. Review generated by Claude Code |
Follow-up Review: Additional Bugs FoundA prior Claude review (#issuecomment-4020945394 through the summary comment) covered the major architecture issues. This comment adds three bugs that were not previously flagged. Bug:
|
PR Review: Finalize split runtime cutoverThis is a significant architectural milestone (14,764 additions / 5,425 deletions across 100 files). Overall the implementation is solid. Logging is comprehensive, actor isolation is correct, and XPC safety patterns are well-designed. Strong pointsHelperManager+RequestHandlers.swift - lib.rs (Rust bridge) - Error handling is thorough, ServiceHealthChecker - Caching RuntimeCoordinator+ServiceManagement.swift - The deliberate removal of legacy fallback ( Potential bugs / races1.
2. The VHID daemon health check and 3. The cutover logic handles the "already running via wrong path" case, but if Kanata is not running at all the function logs and returns. A comment confirming this is by design (UI-driven flow) would prevent future readers from adding auto-start here and reintroducing the legacy path. Design observations4.
5. The returned 6. Both the regular runtime and the passthru runtime use 7. Per CLAUDE.md, be careful about IPC in hot paths. If 8. The timeout Task guards the continuation via Minor / Nits9.
10. Local developer builds get the experimental passthru feature compiled in by default. Consider whether production CI builds should explicitly override this to empty, or document that the default is intentional for local dev. Test coverage
SummaryThe cutover is architecturally sound. The two highest-priority items before merging: (7) verify Generated with Claude Code |
…nd caching - Fix Cargo.toml edition (2024 → 2021) - Add AtomicBool/Mutex to PassthruRuntime for thread-safe state - Replace &mut casts with shared refs (interior mutability) - Assign processing_tx before starting loop thread - Remove TOCTOU preflight TCP bind - Cache SMAppService.status in config reload hot path - Wrap notifyStateChanged in MainActor.run for actor isolation - Use forceRefresh() for fresh permission snapshot - Await bootstrap() before startEventMonitoring() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR 225 Code Review — Finalize split runtime cutoverThis is a substantial architectural cutover (~15K additions, ~5.5K deletions) that makes Critical Bugs1. If only the recovery daemon is running (split runtime failed or has not started yet), 2. When 3. ADR-031 postcondition not enforced on split runtime path — false success possible Per ADR-031, installer/runtime success requires verified runtime readiness ( 4. In 5. Initialization cutover stops the legacy daemon before confirming split runtime starts
Architectural Issues6. The recipe ID says restart but the implementation only regenerates plists without restarting the service. If the TCP comm server is stuck, this silently fails to fix it. Either restart the service or rename the recipe. 7. Install plan gap: VHID healthy but output bridge companion absent
8. Hardcoded
9. Defined in Code Quality10.
11. Debounce threshold hardcoded in
12. Brittle PID assertion after restart in Asserting 13. The lock is released before Minor Suggestions
Overall this is well-structured work with meaningful test expansion. Issues 3 and 5 (ADR-031 postcondition bypass and the stop-before-confirm race) are the most likely to cause zero-remapping failure modes in production and should be addressed before merging. |
|
simple test with single quotes |
…tests The 4 split-runtime tests were calling real pgrep on CI where no Karabiner daemon exists. Add KarabinerConflictService.testDaemonRunning static override (matching existing testPIDProvider / testPersistentHostPID pattern) and set it in the failing tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Finalize Split Runtime CutoverThis is a well-scoped architectural PR promoting the split runtime as the sole normal startup path, removing the legacy process-manager layer, and cleaning up investigation scaffolding. The last commit already addressed several issues from the previous review round. The remaining issues below are ordered by severity. CRITICAL: ADR-031 Postcondition Violation in startKanata()File: Sources/KeyPathAppKit/Managers/RuntimeCoordinator+ServiceManagement.swift (approx. line 102) startKanata() calls KanataSplitRuntimeHostService.shared.startPersistentPassthruHost(includeCapture: true), which returns a PID the moment Process.run() succeeds — before the child process has finished initializing, before the TCP server is bound, and before the output bridge companion is connected. It then returns true immediately. ADR-031 states: "Installer success requires verified runtime readiness (running + TCP responding) before returning success." The PrivilegedOperationsCoordinator install/recover paths correctly call enforceKanataRuntimePostcondition(), but the everyday startKanata() codepath skips all TCP readiness verification. Any caller that interprets the Bool return as "Kanata is fully ready" gets a false positive. Config reloads triggered immediately after start will silently fail. Fix: After startPersistentPassthruHost returns a PID, poll TCP on the configured port with a bounded timeout (matching kanataReadinessTimeout) before returning true. HIGH: Dangling Dylib Handle — dlclose Before PassthruRuntimeHandle Is UsedThe createPassthruRuntime() code loads the dylib with dlopen, allocates a PassthruRuntime heap object inside it, then closes the handle via defer { dlclose(handle) } before returning the KanataHostBridgePassthruRuntimeHandle. The returned handle holds a raw pointer into an unmapped segment. Any subsequent call to start(), sendInput(), or destroy_passthru_runtime operates on a dangling pointer — a live crash risk now that this is the primary runtime path. Fix: Keep the dylib handle open (store it in KanataHostBridgePassthruRuntimeHandle) for the lifetime of the runtime. HIGH: currentRuntimeStatus() Ignores the Legacy Recovery DaemonFile: RuntimeCoordinator+ServiceManagement.swift (approx. line 180) The function checks isPersistentPassthruHostRunning but never consults the legacy recovery daemon. If stopKanata() times out on XPC and the recovery daemon remains active, currentRuntimeStatus() returns .stopped while keys are actively being remapped. Code gating on .stopped will then launch a second runtime, causing a TCP port collision and keyboard freeze. HIGH: Thread-Unsafe activeXPCCalls Set in HelperManager+RequestHandlers.swiftFile: Sources/KeyPathAppKit/Core/HelperManager+RequestHandlers.swift (approx. line 70) activeXPCCalls is read/written with .contains / .insert / .remove without synchronization. executeValueXPCCall spawns a child Task {} (approx. line 143) that escapes the actor, so two concurrent callers can race on the Set. Fix: Ensure the Set is @MainActor-isolated or guard access with an OSAllocatedUnfairLock. MEDIUM: CLAUDE.md Violation — SMAppService.status Called in Tight Poll LoopFile: Sources/KeyPathAppKit/Managers/KanataDaemonManager.swift (approx. line 119) refreshManagementState() is called inside the 500ms poll loop in verifyKanataReadinessAfterInstall(). Each call does synchronous IPC to the ServiceManagement daemon. CLAUDE.md explicitly forbids calling SMAppService.status repeatedly in hot paths ("it does synchronous IPC and can block for 10-30+ seconds under load"). Fix: Cache the status at the start of the verification loop; only re-fetch when a state transition is expected. MEDIUM: isStartingKanata Guard Does Not Protect the Full Async SpanFile: RuntimeCoordinator+ServiceManagement.swift (approx. line 53) startKanata() checks/sets isStartingKanata but suspends at await currentSplitRuntimeDecision(). During that suspension, another caller can enter and see isStartingKanata == false. The ActionDispatcher and RuntimeCoordinator.init Task block also call startPersistentPassthruHost directly, bypassing this guard entirely. MEDIUM: TOCTOU TCP Bind Race in lib.rs — Confirm Fix Applied to run_runtime PathFile: Rust/KeyPathKanataHostBridge/src/lib.rs (approx. lines 447-459) The commit message for 0937189 says "Remove TOCTOU preflight TCP bind," but the keypath_kanata_bridge_run_runtime path appears to still bind a TcpListener and immediately drop it before TcpServer::new. Please confirm the fix was applied to all code paths, not just the passthru path. LOW: sudoExecuteCommand Should Not Be on the Public ProtocolFile: Sources/KeyPathAppKit/Core/PrivilegedOperationsCoordinator.swift (approx. line 22) sudoExecuteCommand(_ command: String, description: String) is declared on the public PrivilegedOperationsCoordinating protocol, making arbitrary shell execution part of the public API contract. CLAUDE.md notes that executeCommand was removed from HelperProtocol for security reasons. The same principle applies here — demote to a private implementation detail. LOW: isRunning Removal Makes UI Blind to Non-Split-Host Active RuntimesFile: Sources/KeyPathAppKit/Models/KanataUIState.swift Removing isRunning and inferring running state from activeRuntimePathTitle != nil means the UI shows a stopped indicator for any Kanata session not originating from the split host (e.g., if the split host fails and Kanata continues via the recovery daemon during cutover). LOW: Silent Failure in Cutover Path of performInitialization()File: Sources/KeyPathAppKit/Managers/RuntimeCoordinator+Lifecycle.swift (approx. line 143) If startKanata() fails during split runtime cutover, the code logs a warning and returns without setting lastError. Users have no indication their session was disrupted. Should set lastError to a user-visible string, consistent with the rest of the codebase. LOW: Triple Crate Type Wastes Build TimeFile: Rust/KeyPathKanataHostBridge/Cargo.toml crate-type = ["staticlib", "cdylib", "rlib"] means three separate compilations per cargo build. The Swift layer only uses cdylib. staticlib is unused by the build scripts; rlib is only needed for cargo test. Consider dropping staticlib from the default targets to cut clean-build time by ~33%. Missing Test Coverage
Summary
The dangling dylib handle (High) and ADR-031 TCP postcondition (Critical) are the two blocking issues before merge. The rest can be addressed in follow-up PRs. |
|
PR Review test - please ignore |
|
hello test2 |
AGENTS.mdThese instructions apply to all Codex sessions in this repository. Web Search Tooling
Rationale: older CLI/tooling may still expose General
Architecture & PatternsInstallerEngine Façade
Permissions
Keyboard Visualization
Service Lifecycle Invariants
Testing
Accessibility (CRITICAL)
Available External ToolsThese CLI tools are available for agents to use on-demand. Do not add them as MCPs - just call them directly when needed. Poltergeist (Auto-Deploy)Watches source files, auto-builds, deploys to /Applications, and restarts. Install:
Workflow tip: Run Peekaboo (UI Automation)macOS screenshots and GUI automation. Install:
KeyPath (Keyboard Control)Trigger via URL scheme: open "keypath://layer/vim" # Switch layer
open "keypath://launch/Safari" # Launch app
open "keypath://window/left" # Snap window
open "keypath://fakekey/nav-mode/tap" # Trigger virtual keyComposing Tools# Example: AI-driven development workflow
poltergeist start # Ensure auto-rebuild is running
# ... make code changes ...
poltergeist wait keypath # Wait for build to complete
peekaboo see "Is the KeyPath app running?" # Check UI state
open "keypath://layer/vim" # Trigger keyboard action
peekaboo type "Hello world" # Type into focused app
peekaboo hotkey "cmd+s" # SaveRecommended agent workflow:
See |
|
REPLACE_BODY_PLACEHOLDER |
ChangelogAll notable changes to this project will be documented in this file. The format is based on Keep a Changelog, and this project follows Semantic [Unreleased]Added
Changed
[0.0.0-internal]Notes
|
|
PR Review test complete - please ignore this comment |
KeyPath Action URI SystemKeyPath supports a URI-based action system that enables:
Syntax FormatsKeyPath accepts two equivalent syntaxes:
Shorthand Syntax (Recommended for Configs)
(push-msg "launch:obsidian") ;; → launches "Obsidian"
(push-msg "layer:nav:activate") ;; → layer "nav", subpath "activate"
(push-msg "notify:?title=Saved") ;; → notification with titleFull URI Syntax (For Deep Links)Used by external tools (Terminal, Raycast, Alfred): open "keypath://launch/Obsidian"
open "keypath://notify?title=Hello&body=World"Supported Actions
|
| Parameter | Description | Default |
|---|---|---|
title |
Notification title | "KeyPath" |
body |
Notification body | "" |
sound |
macOS sound name | (none) |
open:{url} / keypath://open/{url}
Open a URL in the default browser.
Kanata config (shorthand):
(push-msg "open:github.com")
(push-msg "open:https://docs.keypath.app")Notes:
- URLs without a scheme get
https://prepended - URL-encoded characters are decoded automatically
fakekey:{name} / keypath://fakekey/{name}[/{action}]
Trigger a Kanata virtual key (defined via defvirtualkeys or deffakekeys).
Kanata config (shorthand):
;; Define virtual keys in your Kanata config
(defvirtualkeys
email-sig (macro H e l l o spc W o r l d)
toggle-mode (layer-toggle special)
)
;; Trigger from KeyPath
(push-msg "fakekey:email-sig") ;; tap (default)
(push-msg "fakekey:toggle-mode:press")
(push-msg "fakekey:toggle-mode:release")Deep link (for external tools):
open "keypath://fakekey/email-sig/tap"Actions:
| Action | Description |
|---|---|
tap |
Press and immediately release (default) |
press |
Press and hold |
release |
Release a held key |
toggle |
Toggle between pressed and released |
Use cases:
- Trigger macros from external tools (Raycast, Alfred, deep links)
- Execute complex key sequences via simple URL
- Remote-control Kanata layers and modes
Example: Email signature via Raycast
#!/bin/bash
# @raycast.title Insert Email Signature
# @raycast.mode silent
open "keypath://fakekey/email-sig/tap"Kanata Configuration
Naming Conventions
Use full application names in launch aliases, not abbreviations:
| ✅ Recommended | ❌ Avoid |
|---|---|
launch-obsidian |
launch-obs |
launch-terminal |
launch-term |
launch-safari |
launch-saf |
launch-slack |
launch-slk |
launch-visual-studio-code |
launch-vscode |
Why?
- Self-documenting: Anyone reading the config immediately knows which app launches
- Unambiguous:
launch-obscould mean Obsidian or OBS Studio - Consistent: Matches the
launch:{full-app-name}pattern
Basic Usage
Add push-msg to any alias:
(defalias
;; Launch app (use full app name in alias)
launch-obsidian (push-msg "launch:obsidian")
;; Notify on layer switch
nav (multi (push-msg "layer:nav") (layer-switch nav))
;; Track rule usage (optional - consider if you really need it)
caps-escape (multi (push-msg "rule:caps-escape:fired") esc)
)Complete Example
(defcfg
process-unmapped-keys yes
)
(defsrc caps a s d f)
(defalias
;; Caps Lock → Escape (no tracking needed for basic functionality)
caps-escape esc
;; Quick app launchers (use full app name in alias, not abbreviations)
launch-obsidian (push-msg "launch:obsidian")
launch-slack (push-msg "launch:slack")
;; Layer with notification
to-nav (multi
(push-msg "layer:nav:activate")
(push-msg "notify:?title=Nav Mode&sound=Tink")
(layer-switch nav)
)
)
(deflayer base
@caps-escape @launch-obsidian @launch-slack d @to-nav
)
(deflayer nav
@caps-escape left down up right
)Deep Linking from External Tools
Terminal / Shell
open "keypath://launch/Obsidian"
open "keypath://notify?title=Hello&body=World"Raycast
Create a Raycast script command:
#!/bin/bash
# @raycast.title Launch Obsidian via KeyPath
# @raycast.mode silent
open "keypath://launch/Obsidian"Alfred
Create a workflow with "Open URL" action:
- URL:
keypath://launch/{query}
Keyboard Maestro
Use "Open URL" action with keypath:// URLs.
AppleScript
do shell script "open 'keypath://launch/Obsidian'"Architecture
┌─────────────────────────────────────────────────────────────┐
│ Entry Points │
├──────────────────────┬──────────────────────────────────────┤
│ Kanata (push-msg) │ External (open keypath://...) │
│ │ │ │ │
│ ▼ │ ▼ │
│ TCP MessagePush │ URL Scheme Handler │
│ │ │ │ │
│ ▼ │ ▼ │
│ KanataEventListener │ AppDelegate.application(_:open:)│
│ │ │ │ │
└─────────┴────────────┴──────────────┴────────────────────────┘
│
▼
┌─────────────────┐
│ KeyPathActionURI │ (Parses keypath:// URIs)
└────────┬────────┘
│
▼
┌─────────────────┐
│ ActionDispatcher │ (Routes to handlers)
└────────┬────────┘
│
┌─────────┬───────┼───────┬─────────┐
▼ ▼ ▼ ▼ ▼
handleLaunch handleNotify handleOpen handleFakeKey
│
▼
┌──────────────┐
│KanataTCPClient│
│ ActOnFakeKey │
└──────────────┘
Custom Handlers
Subscribe to ActionDispatcher callbacks for custom handling:
// In your code
ActionDispatcher.shared.onLayerAction = { layerName in
// Custom layer change handling
print("Layer changed to: \(layerName)")
}
ActionDispatcher.shared.onRuleAction = { ruleId, path in
// Custom rule tracking
Analytics.track("rule_fired", properties: ["rule": ruleId])
}
ActionDispatcher.shared.onError = { message in
// Show error toast/dialog
showToast(message)
}Error Handling
| Scenario | Behavior |
|---|---|
| Unknown action type | Logs warning, calls onError callback |
| Missing target | Logs warning, calls onError callback |
| App not found | Attempts all search paths, then calls onError |
| Invalid URL format | Logs warning, calls onError callback |
Security Considerations
- Launch action: Only launches apps from standard macOS app directories
- Open action: Opens URLs in user's default browser (sandboxed)
- No file system access: Actions cannot read/write arbitrary files
- No shell execution:
cmdaction is not supported (use Kanata's nativecmdfor that)
Virtual Keys Inspector
KeyPath includes a built-in inspector for viewing and testing virtual keys defined in your configuration.
Accessing the Inspector
- Open KeyPath Settings (⌘,)
- Go to the General tab
- Scroll down to the Virtual Keys section
Features
| Feature | Description |
|---|---|
| Key List | Shows all keys from defvirtualkeys and deffakekeys blocks |
| Copy URL | Copy the deep link URL to clipboard for use in Raycast, Alfred, etc. |
| Test Button | Trigger the virtual key immediately to verify it works |
| Refresh | Re-parse the config file to pick up changes |
Requirements
- Virtual keys must be defined in your Kanata config using
defvirtualkeysordeffakekeys - Kanata service must be running to test keys (TCP connection required)
- The inspector is read-only; edit your config file to add/modify keys
- Feature flag
SIMULATOR_AND_VIRTUAL_KEYS_ENABLEDmust be enabled to access the inspector and trigger virtual keys
Troubleshooting
| Issue | Solution |
|---|---|
| "No Virtual Keys Defined" | Add defvirtualkeys or deffakekeys blocks to your config |
| "Network error" on test | Ensure Kanata service is running |
| Key not triggering | Verify the key name matches exactly (case-sensitive) |
| Changes not showing | Click Refresh or reopen Settings |
Future Extensions
Planned action types (not yet implemented):
keypath://clipboard/copy?text=...- Copy text to clipboardkeypath://sound/play?name=...- Play a soundkeypath://window/focus?app=...- Focus specific windowkeypath://shortcut/run?name=...- Run macOS Shortcut
|
PR Review: Finalize Split Runtime Cutover - See full review at #225 |
PR Review: Finalize Split Runtime CutoverThis is a large, well-structured refactor. The overall direction is sound: making the split runtime the primary path and reserving legacy daemon paths for recovery only. Below is a mix of bugs, concerns, and observations across the key changed areas. Bugs / Correctness Issueslib.rs - TOCTOU race in keypath_kanata_bridge_run_runtime The function binds a TcpListener to verify the port, drops it immediately, then lets TcpServer::new bind again. Another process can steal the port in that window. The probe-then-bind pattern does not protect against anything here -- it introduces the race. Either skip the probe and let TcpServer::new return an error, or hold the listener and hand it to the server. lib.rs - C++ mangled symbol link The bridge links by a hardcoded C++ name-mangled symbol (__Z9init_sinkv). It will silently break if the linked library is rebuilt with a different ABI, compiler, or optimization settings. Consider exporting the sink initializer with extern C on the C++ side and linking the stable symbol instead. write_error - silent truncation When an error string exceeds the buffer, the caller gets a partial message with no indication it was cut. Consider appending ... on truncation, or adding a debug assertion that callers must use sufficiently large buffers. RuntimeCoordinator+ServiceManagement.swift - confirm isStartingKanata lifecycle currentRuntimeStatus() returns .starting when isStartingKanata is set. Verify that startKanata() actually sets and clears this flag around the async start path. If the flag is only set from a separate call site, the .starting state may never be returned and the UI will appear stuck in .stopped during startup. Architectural / Design ConcernsServiceHealthChecker - redundant launchctl calls within a validation cycle CLAUDE.md warns explicitly against calling SMAppService.status repeatedly in a hot path. The same risk applies to launchctl print. checkKanataServiceRuntimeSnapshot() (the no-args overload) calls KanataDaemonManager.shared.refreshManagementState() then delegates to the two-arg overload which calls evaluateKanataLaunchctlRunningState again. If a validation cycle also calls isServiceLoaded + isServiceHealthy, management state ends up fetched 3-4 times. A per-cycle cache struct would address this, consistent with ADR-026 intent. decideKanataHealth - inputCaptureReady masks staleRegistration diagnosis If Kanata logs an IOHIDDeviceOpen error and the registration is stale, the user sees kanata-cannot-open-built-in-keyboard when the real actionable issue is stale-enabled-registration. Consider checking staleEnabledRegistration first -- a stale registration cannot capture input anyway, so surfacing it first is more useful for recovery. checkKanataInputCaptureStatus - fragile string matching on stderr The health decision depends on specific substrings in Kanata stderr log. This breaks silently if Kanata changes its log format, and reads up to 64 KB on every health check invocation. The inputCaptureStatusOverride test seam in DEBUG mode is good; consider adding a comment tying the matched strings to the specific Kanata version range where they were validated. build-kanata-host-bridge.sh - missing MACOSX_DEPLOYMENT_TARGET build-kanata-runtime-library.sh sets MACOSX_DEPLOYMENT_TARGET=11.0 but the bridge build script does not. Without it, the Rust toolchain may emit symbols requiring newer OS versions, causing linker warnings or errors on the Swift side. Mirror the deployment target from the other script. executeValueXPCCall - continuation asymmetry with executeXPCCall In executeXPCCall the proxy is fetched before the continuation. In executeValueXPCCall the proxy is fetched inside a nested Task inside the continuation. If the timeout fires and resumes the continuation while the proxy Task simultaneously calls completionState.tryComplete(), the guard prevents double-resume but the proxy Task error is silently swallowed. Prefer the executeXPCCall pattern: fetch the proxy before entering the continuation and make the XPC call synchronously inside it. Positive ObservationsCompanion health monitor (checkSplitRuntimeCompanionHealth + startSplitRuntimeCompanionMonitor) is cleanly structured with an isRecoveringSplitRuntimeCompanion guard against re-entrant recovery. HelperXPCCallCompletionState with NSLock is correct and avoids the common Bool race. KanataHealthDecision.transient for TCP warmup after restart is a well-named pattern that avoids false-positive unhealthy alerts during restart windows. Deleting ProcessManager.swift and ProcessCoordinator.swift is the right move. DEBUG test seams (runtimeSnapshotOverride, recentlyRestartedOverride, inputCaptureStatusOverride) give good coverage hooks without polluting production paths. Minor / PolishCargo.toml declares crate-type with staticlib, cdylib, and rlib. Producing all three from the same crate increases build time. The build script only uses the static library; the cdylib entry can be dropped unless it is needed elsewhere. Confirm the External/kanata submodule pointer bump is intentional and points to the correct upstream commit for this bridge version. isOneShotProbeMode skips MainAppStateController.shared.configure(with: manager) correctly, but still initializes TypingSoundsManager.shared. That singleton may do unnecessary work in probe mode. SummaryMain items to address before merge: the TOCTOU port race in lib.rs, the missing MACOSX_DEPLOYMENT_TARGET in the bridge build script, and the executeValueXPCCall continuation asymmetry. The decideKanataHealth ordering and redundant launchctl call concerns are lower urgency but worth tracking to avoid reintroducing the slow-path issues the ADRs documented. |
1 participant
Summary
Validation
Notes