feat(health): consume kanata's authoritative InputGrab status (#630)

[malpern]

Summary

Wires KeyPath to consume the bundled kanata fork's new authoritative InputGrab TCP status as the primary, VNC-immune input-capture health signal. Closes the consumer half of #630.

Today kanata's health is entirely inferential — process + TCP + stderr log-scrape + "are KeyInput events flowing." The functional check is ambiguous: no key events can mean kanata-not-grabbing, user-not-typing, or user-on-VNC (synthetic events bypass the physical grab — the long misdiagnosis from last session). kanata itself knows the ground truth: it just succeeded or failed at seizing the keyboard. The fork now emits that over TCP ({"InputGrab":{"active":bool,"devices":[...],"reason":...}}, commit cd2a1e5 on keypath/bundled); this PR consumes it.

What's here

• KanataGrabStatusStore (KeyPathCore): process-wide store of the latest InputGrab, recorded by the TCP listener and reset when the connection drops, so a non-nil value is always ground truth from the current live session.
• KanataEventListener: parses the InputGrab message and records it (parseInputGrab is a unit-testable static helper, matching the existing extractMessagePushMessages / normalizedCapabilities pattern).
• ServiceHealthChecker.resolveInputCaptureStatus: layers the signal over the fix: detect kanata 'up but not grabbing the keyboard' so status stops lying (#624) #632 stderr detector, strictly additive — only an authoritative active:false overrides the fallback; active:true never suppresses a stderr-detected failure (the grab bit is coarser than stderr; a cached active:true could outlive a silent grab loss). A recovery active:true clears a prior failure by deferring to the now-clean stderr. Wired into both health pipelines (checkKanataServiceRuntimeSnapshot and SystemValidator.checkHealth) so they agree.
• Submodule pin bumped to cd2a1e5.

Design notes (belt-and-suspenders)

The authoritative signal can only make status more truthful (catch a VNC-masked failure stderr missed), never less. kanata does not replay InputGrab on connect, so on a fresh connect to an already-grabbing kanata the store stays empty and the #632 stderr detector remains the backstop — exactly the intended layering.

Review gate

Ran the mandatory adversarial review (correctness + concurrency + cleanup/altitude angles). It caught two real issues that are fixed in this PR:

1. Half-wired path — SystemValidator.checkHealth (a primary production pipeline feeding SystemSnapshot/SystemInspector/InstallerEngine) bypassed the new signal. Now routed through the same chokepoint.
2. Status could become less truthful — an earlier version let active:true mask a stderr-detected failure / go stale. Fixed by making the override active:false-only (strictly additive).
   Also removed a speculative unused NotificationCenter broadcast the review flagged.

Testing

• New KanataInputGrabConsumerTests (parse, store record/reset, resolver precedence incl. the no-mask + recovery invariants, end-to-end decision).
• Existing KanataInputGrabDetectionTests (fix: detect kanata 'up but not grabbing the keyboard' so status stops lying (#624) #632) and ServiceHealthCheckerTests still green.
• Full XCTest suite passes (the swift-testing SIGTRAP flake is pre-existing — reproduces on a clean tree without these changes).

Follow-ups

• Health status reports green while kanata input is non-functional (degraded keyboard grab undetected) #624 (honest VNC-aware status UI) and kanata restart can leave input degraded: keyboard-grab release race + no grab-failure detection/retry #625 (auto-recovery) build on this.
• Upstreaming the fork side to jtroo/kanata is tracked in malpern/kanata#20 (needs Linux/Windows emit sites + a RequestInputGrab query path).

🤖 Generated with Claude Code

[claude]

Code Review — feat(health): consume kanata's authoritative InputGrab status (#630)

Overview

Solid, well-motivated PR. The belt-and-suspenders layering design is correct: active:false is authoritative, active:true never suppresses a stderr-detected failure, and the store resets on connection drop so stale status can never outlive its session. The review-gate self-corrections (wiring SystemValidator.checkHealth and removing the active:true mask) are visible and real improvements.

---

Issues

1. KanataGrabStatusStore.init() should be private

KanataGrabStatusStore is a singleton but exposes a public init(), letting callers create independent instances that bypass the shared store. Any code accidentally constructing a local instance would silently write to a dead store rather than failing at compile time.

// Current — allows: let s = KanataGrabStatusStore(); s.record(...)
public init() {}

// Should be:
private init() {}

2. observedAt field is dead weight in the current health logic

KanataInputGrabStatus.observedAt is stored and logged, but resolveInputCaptureStatus never reads it. The PR description argues that a cached active:true "could outlive a silent grab loss" — so the reset-on-disconnect approach is the staleness guard, not timestamp expiry. That design is fine, but then observedAt is misleading public API that implies staleness checks happen (or will happen). Options:

• Keep it but add a note that it's for diagnostics/logging only, not used in health decisions
• Remove it now and add it back if/when an age-out policy is implemented
• Use it: e.g., in resolveInputCaptureStatus, ignore an active:false older than N seconds (though the reset-on-disconnect design may make this redundant)

3. Test class doesn't extend KeyPathTestCase

KanataInputGrabConsumerTests extends XCTestCase directly. The CLAUDE.md rule ("use KeyPathTestCase for any test that uses InstallerEngine, RuntimeCoordinator, SystemValidator, VHIDDeviceManager") technically doesn't apply here since these tests only call pure static functions and manipulate the grab store directly — no process-spawning paths. But:

• testDecideHealth_inactiveGrab_marksUnhealthy constructs a KanataServiceRuntimeSnapshot with managementState: .smappserviceActive — currently safe, but if that enum value ever lazily touches a live path the test will silently start deadlocking
• Extending KeyPathTestCase costs nothing and would call TestSingletonReset.resetAll() in setUp/tearDown, which is safer isolation for a test that mutates a global singleton (KanataGrabStatusStore.shared)

Recommend extending KeyPathTestCase for consistency and defensive isolation.

---

Minor Notes

• Two reset() call sites in KanataEventListener (lines 431 and 469): both are correct and idempotent. Worth a drive-by check that there isn't a third disconnect path (e.g. an unhandled error before listenLoop reaches its teardown). A quick grep shows only these two, so this appears complete.

• stderrFallback parameter name in resolveInputCaptureStatus: accurate, but callers pass stderrDiagnosis.inputCapture — worth ensuring new callers don't accidentally pass the entire DaemonStderrDiagnosis instead of .inputCapture. The type system catches this; just noting for reviewer awareness.

• Comment density: Comments on the new code explain genuine non-obvious invariants (VNC immunity, no-replay-on-connect, coarseness of the grab bit). Justified given how subtle the layering is. The block comment on resolveInputCaptureStatus is long but the reasoning it documents is exactly the kind of thing that will be re-litigated in six months without it.

---

Summary

The two actionable items are the private init() fix (simple, prevents a real footgun) and the observedAt clarification (design question about whether timestamp-based staleness is a future concern). The KeyPathTestCase note is a consistency recommendation, not a correctness issue. Everything else looks sound — the locking model is correct, both health pipelines are wired, and the test coverage maps cleanly to the stated design invariants.

🤖 Generated with Claude Code

[claude]

Code Review

Overall: Well-designed PR that correctly implements the "strictly additive" signal-layering invariant. Architecture is sound, test coverage is good. One real bug to fix before merge; a few minor points.

---

Bug: Log line in SystemValidator reports the wrong value

SystemValidator.swift:520 still logs stderrDiagnosis.inputCapture.isReady after the resolved value was moved to inputCapture:

// What the log currently says:
"inputCaptureReady=\(stderrDiagnosis.inputCapture.isReady)"  // ← stderr-only value

// What the health decision actually uses (lines 557-558):
kanataInputCaptureReady: inputCapture.isReady  // ← resolved value (may differ!)

In the exact scenario this PR is designed to catch (VNC-masked grab failure: stderr looks clean, InputGrab.active=false), the log would show inputCaptureReady=true while the snapshot records false. Any debugging session for a VNC/grab failure would see a misleading log and waste time. Should be:

"inputCaptureReady=\(inputCapture.isReady)"

---

Minor: Double-reset paths in KanataEventListener

KanataGrabStatusStore.shared.reset() is called from both cleanupAfterDisconnect and the connection-drop path inside listenLoop. Worth confirming cleanupAfterDisconnect is not always called after listenLoop's drop path (double-reset is idempotent, but redundant resets are noise). If one path always subsumes the other, the redundant call could be removed.

---

Minor: Comment verbosity

Several new comment blocks are longer than the project style typically favors (CLAUDE.md: "Only add [a comment] when the WHY is non-obvious"). The two-site reset comments in KanataEventListener are justified (non-obvious timing). The long doc-block on resolveInputCaptureStatus could be trimmed — the invariant is already expressed clearly in the guard clause itself. Not a blocking concern.

---

What's done well

• resolveInputCaptureStatus logic is exactly right. The guard let grab = ..., !grab.active else { return stderrFallback } idiom correctly implements active:false-only override without the asymmetric staleness risk.
• Store reset on connection drop (both paths) prevents a stale active:true from outliving a crash. The structural staleness model (reset-on-drop vs. age-out) is the right call.
• parseInputGrab as a static helper follows the existing extractMessagePushMessages/normalizedCapabilities pattern and makes it trivially unit-testable.
• Test suite is solid. Covers parse edge cases, store record/reset, no-mask invariant, recovery transition, default issue string, and end-to-end decideKanataHealth. GlobalTestSetup reset ensures no bleed between suites.
• KanataGrabStatusStore thread-safety via NSLock.withLock + @unchecked Sendable is the established pattern in this codebase and is correct here.

---

Summary

Fix the log line on SystemValidator.swift:520 (stderrDiagnosis.inputCapture.isReady → inputCapture.isReady) and this is ready to merge.

🤖 Generated with Claude Code

[claude]

Code Review — feat(health): consume kanata's authoritative InputGrab status (#630)

Overview

This PR closes the consumer half of the VNC-immune health signal story: kanata now emits InputGrab over TCP and KeyPath reads it as a strictly-additive override on top of the existing stderr log-pattern detector. The design is sound — only active:false can make status worse, active:true can never suppress a stderr-detected failure, and the store is reset on TCP disconnect so stale values can't outlive their session.

---

Correctness ✅

resolveInputCaptureStatus guard logic is correct (#630):

guard let grab = KanataGrabStatusStore.shared.latest, !grab.active else {
    return stderrFallback
}

Exits (falls back) on nil store or active:true, only falls through to a failure return when active:false. This is the intended belt-and-suspenders behaviour. The local binding on grab is captured before any reset could fire, so this is TOCTOU-safe.

Reset sites are correct and idempotent. Both cancelListening() and the listenLoop() drop-path reset the store, meaning both an explicit cancel and a dropped connection clear stale state. Since reset() is idempotent (nil → nil), double-reset on concurrent cancel+drop is harmless.

Both health pipelines are now wired. The fix to route SystemValidator.checkHealth through resolveInputCaptureStatus (the half-wired path caught in the mandatory review gate) is the right call — it ensures SystemSnapshot/SystemInspector/InstallerEngine all see the same signal.

---

Issues / Suggestions

1. KanataInputGrabStatus.Equatable includes observedAt: Date (minor)

KanataInputGrabStatus derives Equatable by synthesis, so observedAt participates in ==. Two identical grab states observed at different times will compare unequal. This is fine for the current tests (they use the same status binding), but if any future code asks "did the grab state change?" by comparing values, it will always answer "yes" even when only the timestamp changed.

Suggestion: Either exclude observedAt from the synthesized conformance via a custom ==:

public static func == (lhs: Self, rhs: Self) -> Bool {
    lhs.active == rhs.active && lhs.devices == rhs.devices && lhs.reason == rhs.reason
}

or document the timestamp-inclusive equality as intentional. Not urgent, but worth locking in the semantics now.

2. Default-issue string duplicated across two sites (minor)

"kanata-failed-to-grab-keyboard" appears once in resolveInputCaptureStatus and once in the test assertion. A file-private (or module-internal) constant would prevent drift:

// In KanataGrabStatus.swift or ServiceHealthChecker.swift
static let defaultGrabFailureIssue = "kanata-failed-to-grab-keyboard"

Low priority, but it's the kind of string that quietly diverges.

3. Comment verbosity slightly exceeds CLAUDE.md guidance

CLAUDE.md: "Only add a comment when the WHY is non-obvious." The TCP format-spec comments in handleMessage are valuable (they document the wire protocol). The multi-paragraph docstring on resolveInputCaptureStatus largely repeats the PR description — the WHY (VNC ambiguity, strictly additive) is genuinely non-obvious and belongs somewhere, but most readers will be confused by a why-comment that's longer than the function body. Consider trimming to the one-line "strictly additive: only active:false overrides; active:true falls back to stderr" and linking to the PR/ADR.

4. @MainActor on test class (question, not a bug)

KanataInputGrabConsumerTests is @MainActor. The tests only call thread-safe statics and the singleton (which uses NSLock). Is @MainActor required here, or is it inherited from the base class? If it's just inherited convention, that's fine. If it's unnecessary, removing it would let the tests run off the main thread and potentially catch concurrency issues the lock is meant to protect against.

---

Missing Test Coverage

• Concurrent record/reset: No test exercises concurrent calls to the store. Given the NSLock implementation this should be fine, but a simple concurrent dispatch test would catch accidental lock removal in a refactor. Optional, but low cost.
• cancelListening reset path: The test suite verifies the store behaviorally but doesn't exercise the reset that fires inside KanataEventListener.cancelListening() / listenLoop() drop path directly. The TestSingletonReset global cleanup covers bleed-between-tests, but a test that simulates a dropped connection resetting the store would document the session-boundary invariant explicitly.

---

Security / Performance ✅

• JSON parsing is defensive (guard on active: Bool, graceful defaults for devices and reason). The reason string is used only for display, not executed — no injection risk.
• NSLock.withLock is appropriate for this access pattern; the lock is held only for simple assignments, never across async suspension points.
• No hot-path concerns: resolveInputCaptureStatus is called from async health checks, not from event dispatch.

---

Summary

Solid PR. The design is well-reasoned, both health pipelines are now consistent, the strictly-additive override logic is correct, and the test suite covers the key invariants cleanly. The items above are minor polish — the Equatable/observedAt point is the one worth resolving before this pattern spreads to comparison sites.

Verdict: Approve with the Equatable semantic question addressed (either fix or document).