Fix CLI service lifecycle through SMAppService

[malpern]

Summary

• route CLI service start, stop, and restart through the owning SMAppService lifecycle
• verify unregister removes both the registration and process, with a bounded retry and helper-backed stale-job cleanup
• bump the 1.0.1 release candidate to build 12 and document the live acceptance finding

Validation

• 16 focused lifecycle/CLI tests passed
• Developer ID signed build 12 passed the identity contract and strict codesign verification
• live build 11 to build 12 replacement refreshed the registered runtime to build 12
• one stop remained down across five observations over 10 seconds
• one start returned operational with build 12, helper 1.3.2, and VirtualHID 8 healthy

Release scope

This is the final lifecycle correction required before the 1.0.1 Sparkle release candidate is rebuilt and notarized. Physical HID proof remains a separate lab gate and is not claimed here.

[claude]

Two points worth a look in KanataDaemonService.swift:

1. Possible slow/hanging stop() via repeated hot-path SMAppService status queries. stop() can call waitForStoppedPostcondition() up to three separate times (default 10 attempts, default 10 attempts, then a final call with maxAttempts: 20) — up to ~40 loop iterations in the worst case, each invoking stoppedPostconditionSatisfied() → SystemStateProvider.shared.freshSMAppServiceStatus(...). This project's own guidance (AGENTS.md/CLAUDE.md) explicitly warns: "Do not call SMAppService.status in a hot path; it is synchronous IPC and can block for 10-30 seconds." If freshSMAppServiceStatus ultimately queries SMAppService.status directly (its name implies a cache bypass), a worst-case stop()/restart() call could block for many minutes rather than the ~4s the 100ms delays suggest, effectively hanging the CLI/service-control command. Worth confirming SystemStateProvider.freshSMAppServiceStatus offloads/bounds this call, or reducing how many times the postcondition is independently re-polled.

2. Asymmetric postcondition verification between stop() and start(). stop() now does thorough postcondition verification (registration + process state) with retries and a privileged fallback before reporting success. start() (registerDaemon() + cache invalidation) reports success immediately after service.register() succeeds, with no verification that the daemon actually reaches a running/responding state. This is the same class of issue this repo's own docs flag: "Registration is not liveness... never infer runtime health from [SMAppService status] without process + TCP evidence" and "Mutating installer actions must be postcondition-verified before returning success." If the equivalent readiness wait isn't guaranteed by the caller (e.g. SystemFacade/CLI layer), start()/restart() could report success on a service that never actually reaches running/responding state. Worth confirming that guarantee lives elsewhere, or adding a matching wait here for consistency with stop().

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8ed17089f

ℹ️ 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]

Recover stale enabled registrations before polling

When SMAppService.status is .enabled but the daemon is no longer loaded or running, calling register() directly does not perform the existing unregister/re-register recovery or kickstart implemented by KanataDaemonManager.register(). In that degraded state, keypath-cli service start will poll for 20 seconds and return false while leaving Kanata stopped; inspect the fresh registration state and route this through the canonical recovery path instead.

AGENTS.md reference: AGENTS.md:L313-L316

Useful? React with 👍 / 👎.

[claude]

Correctness/reliability gap: start()/restart() are not postcondition-verified, unlike stop()

KanataDaemonService.stop() (Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift) now does extensive verification — waitForStoppedPostcondition() polling registration + process state, a retry of unregisterDaemon(), and a privileged fallback via forceStopStaleDaemon() — before it will report success.

start() has no equivalent:

func start() async throws {
    try await registerDaemon()
    await pidCache.invalidateCache()
    lastObservedState = .unknown
    AppLogger.shared.info("✅ ... Start requested successfully")
}

It only checks that SMAppService.register() didn't throw — it never confirms the daemon actually reached a running state (process alive / TCP responding), and doesn't handle SMAppService.status == .requiresApproval, which register() can return without throwing (e.g., user hasn't approved the login item in System Settings). In that case start() logs "Start requested successfully" while Kanata never actually launches.

restart() is stop() followed by start(), so it inherits this: a caller can get a "successful" restart where the daemon never comes back up.

This directly conflicts with the repo's own invariant (AGENTS.md): "Mutating installer actions must be postcondition-verified before returning success... any action that can stop/restart/re-register Kanata must verify runtime readiness (running + TCP responding) or explicit pending-approval state before reporting success." It's also the same class of bug this PR's linked doc (docs/bugs/cli-service-control-helper-bypass.md) is fixing for stop — worth closing the loop for start/restart too, especially since SystemFacade now calls KanataDaemonService.start()/restart() directly.

Test coverage reflects the gap: testStartServiceRegisters and testRestartServiceUnregistersBeforeRegistering only assert register/unregister were called, not that any running-state postcondition was checked — there's no test for the .requiresApproval or "register succeeded but process never came up" cases.

No other blocking issues found in the diff.

[malpern]

Addressed both lifecycle review findings in 69a0c2a:

• Stop now performs one fresh SMAppService status read per bounded cleanup phase and polls only launchd/process evidence between phases, rather than issuing fresh status IPC on every 100 ms iteration.
• Start now distinguishes pending approval, failed registration persistence, and failed process/TCP readiness; it cannot report success until registration is enabled and runtime readiness is proven. Restart inherits the same verified start contract.
• Added explicit approval-required and registered-but-not-ready tests. The focused CLI/lifecycle suite is now 18/18 green, and pinned SwiftFormat passes.

[claude]

Reviewed the diff. One likely correctness bug plus a test-coverage gap in KanataDaemonService.stop():

Bug: final postcondition check can gate forever on stale SMAppService registration, even after the privileged fallback succeeds.

waitForStoppedPostcondition() (no override) does:

let status = await currentRegistrationStatus()
guard status == .notRegistered || status == .notFound else { return false }
// only then polls process state

stop() calls this after forceStopStaleDaemon() too:

guard await waitForStoppedPostcondition(maxAttempts: 20) else {
    throw KanataDaemonServiceError.stopFailed(...)
}

But per this PR's own doc update, the privileged helper fallback "no longer tries to emulate the owning app's SMAppService lifecycle" — i.e. it kills the process without touching SMAppService registration. If macOS is slow to reflect the unregister (the exact stale-job scenario this PR targets), currentRegistrationStatus() can still report .enabled indefinitely, so the guard short-circuits to false without ever checking whether the process actually stopped, and stop()/restart() throw stopFailed even though the daemon was successfully killed via the privileged path. Consider having the final check accept "process confirmed stopped" as sufficient regardless of lingering registration status, or retry the registration read instead of a single fresh snapshot.

Test gap: testStopRetriesUnregisterThenUsesPrivilegedFallbackForStaleJob sets stoppedPostconditionOverride, which bypasses the real status-gating branch above entirely. None of the new tests exercise the production waitForStoppedPostcondition path (real currentRegistrationStatus() check + process poll), so the bug above wouldn't be caught by this suite.

Minor perf note: start()/stop() can now issue up to 2–3 fresh synchronous SMAppService.status IPC calls per call (currentRegistrationStatus()), each documented elsewhere in this repo as potentially blocking 10–30s. Worth confirming CLI/UI callers tolerate the worst-case added latency versus the prior single-check implementation.

[malpern]

Addressed the follow-up stop-state concern in 88f470aa4. The postcondition now distinguishes registration still present from a removed registration with a lingering process. Privileged cleanup is followed by another owning-API unregister only when registration remains; the final gate still requires both registration removal and process exit, so a merely killed process cannot be reported as a durable stop. The tests now exercise the real registration/process branches (no composite postcondition bypass), including stale registration and process-only lingering cases. Focused suite: 19/19 green; pinned formatting: green.

[malpern]

Correction: the follow-up fix commit is 928dae5 (the behavior and 19/19 validation described above are unchanged).

[claude]

Reviewed the diff. One reliability concern in the new stop() postcondition logic in KanataDaemonService.swift:

Asymmetric/short timeout before escalating to the privileged helper. waitForStoppedPostcondition() defaults to maxAttempts: 10, delayMilliseconds: 100 (1s total) for process-exit evidence. In stop(), if the first check comes back .processRunning (registration removed but process still exiting), the code skips the retry-unregister branch entirely and escalates straight to forceStopStaleDaemon() (the privileged helper bootout) after only that ~1s window. Compare this to waitForRunningPostcondition() used by start(), which allows up to 80 × 250ms = 20s for the process/TCP to become ready.

If kanata's normal shutdown (releasing HID resources, etc.) routinely takes more than ~1s, this will make stop() invoke the privileged helper on every ordinary stop rather than only in the "stale job across an app replacement" scenario the code comments and docs/bugs/cli-service-control-helper-bypass.md describe — which undercuts the stated goal of relying on SMAppService ownership instead of helper-based teardown for routine operation. testStopUsesPrivilegedFallbackWithoutRedundantUnregisterWhenOnlyProcessLingers confirms this is the current tested behavior, so it appears intentional, but worth confirming against real-world kanata shutdown latency before merging — otherwise consider giving the initial waitForStoppedPostcondition() call a longer/more comparable timeout before falling back to the privileged path.

No other correctness issues found; the SMAppService register/unregister state machine, .enabled-is-not-liveness handling in start(), and the new test coverage otherwise look sound.

[malpern]

Addressed the shutdown-grace concern in 76a85e1: ordinary SMAppService stop now waits six seconds, slightly longer than the launchd plist's five-second exit timeout, before invoking privileged cleanup. Start retains its 20-second readiness window. The real lingering-process fallback test now exercises that full grace period; focused suite remains 19/19 green and pinned formatting passes.

[claude]

One correctness inconsistency worth a look in KanataDaemonService.stop() (Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift):

In the escalation path, the first unregister retry swallows errors and re-polls the real postcondition:

try? await unregisterDaemon()
stoppedPostcondition = await waitForStoppedPostcondition()

but the unregister issued right after the privileged fallback succeeds does not:

try await forceStopStaleDaemon()
...
if stoppedPostcondition == .registrationPresent {
    try await unregisterDaemon()   // <-- unguarded here
}
stoppedPostcondition = await waitForStoppedPostcondition(maxAttempts: 20)

If that last unregister throws a transient/benign error (e.g. an SMAppService IPC hiccup, or the job briefly reporting "not found" right after the privileged bootout), stop() aborts immediately with .stopFailed even though the privileged cleanup may have already accomplished the goal and the subsequent postcondition poll would likely have confirmed it. This breaks the "verify the real postcondition, don't trust individual API calls" philosophy the rest of the function follows, and it's not covered by the new tests — MockSMAppService.unregister() never throws, so this path isn't exercised. Suggest using try? here as well (or explicitly catching/mapping the error the same way forceStopStaleDaemon()'s own errors are handled just above), and adding a test where this final unregisterDaemon() throws but the postcondition would otherwise be satisfied.

Everything else in the diff (the new start()/registration-status handling, the stopped/running postcondition polling, the SystemFacade wiring change) looks sound and is reasonably well covered by the new integration tests.

[malpern]

Addressed the transient post-fallback unregister concern in ec081c0: that cleanup call is now best-effort, and the final registration+process postcondition remains authoritative. Removed the test-only unregister-error suppression and added a test where the third unregister throws after cleanup but the real final state is healthy. Focused suite: 20/20 green; pinned formatting: green.

[claude]

Reviewed the diff. One correctness concern in KanataDaemonService.stop() (Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift):

The method opens with an unguarded try await unregisterDaemon(). If that first call throws (rather than succeeding while leaving a stale registration), stop() propagates the error immediately and never reaches the new waitForStoppedPostcondition retry / forceStopStaleDaemon() privileged-fallback logic that this PR adds specifically to handle exactly the "job survives across app replacement" scenario described in docs/bugs/cli-service-control-helper-bypass.md. Since restart() is just try await stop(); try await start(), the same gap means a transient/legitimate unregister() exception on restart's stop phase skips all the new resiliency and fails the whole restart outright — even though the point of this change is to make stop/restart robust to exactly that kind of stale-job weirdness.

All the other unregister calls in the method are wrapped in try? (so failures fall through to the postcondition check), but the first one isn't, and no test exercises "initial unregister() throws" — the new tests (testStopRetriesUnregisterThenUsesPrivilegedFallbackForStaleJob, testStopVerifiesFinalStateWhenPostFallbackUnregisterThrows) only cover cases where the first unregister succeeds but leaves stale registration state.

Suggest wrapping the initial call in try? (or catching and falling into the postcondition check) so a thrown error from the first unregisterDaemon() gets the same retry/privileged-fallback treatment as a silently-stale registration, plus a test for that path.

Everything else (start()'s registration/approval/readiness handling, the stopped/running postcondition polling, the DEBUG-only test overrides) looks sound and well covered by the new integration tests.

[malpern]

Addressed the initial-unregister concern and completed the same state-authoritative contract for all lifecycle mutations in 888372d: initial unregister, register, and privileged cleanup delivery errors no longer preempt a fresh final-state check. Added coverage for initial unregister failure, register-throws-after-enabling, and privileged-cleanup error with successful final recovery. Focused suite: 23/23 green; pinned formatting: green.

[claude]

Reviewed the diff. The new SMAppService-owned start/stop/restart lifecycle in KanataDaemonService.swift is well covered by the added integration tests (registration retry, privileged-fallback ordering, postcondition verification), and the design correctly avoids calling SMAppService.status inside a polling loop (fetched once per phase, per the existing hot-path guidance).

Two non-blocking points worth confirming before merge:

1. Compounded worst-case latency in restart(). restart() is now stop() followed by start(), and each has its own multi-stage retry/fallback path: stop() can spend up to ~6s waiting on the initial postcondition, retry unregister (another ~6s ceiling), then fall back to the privileged bootout plus a final ~2s wait; start() separately polls up to 80×250ms (~20s) for process+TCP readiness. In pathological cases (stale job + slow launch) restart() could take on the order of 30-40s, versus the old single kickstart -k call. Worth checking that CLI/UI callers of SystemFacade.restartServiceOperation (and any wizard/repair flows that call through KanataDaemonService) have timeouts sized for this, rather than assuming the old fast-path duration.

2. New KanataDaemonServiceError cases. .approvalRequired and .startFailed are new. Worth double-checking any place outside this diff that pattern-matches on KanataDaemonServiceError (CLI error output, wizard UI, installer repair flows) surfaces .approvalRequired with an actionable "open System Settings" message rather than falling through to a generic failure string — that case in particular is user-actionable and worth a distinct UX path.

No correctness bugs found in the postcondition/retry state machine itself — the .registrationPresent vs .processRunning branching in stop() and the fresh-status-after-register-error handling in start() both look sound and match the new tests.

[malpern]

Final integration confirmation:

• Callers await the lifecycle operation; SystemFacade then independently verifies the final runtime state. The bounded worst-case retry window does not violate a caller timeout or assume an immediate kickstart.
• CLI start/stop/restart failures are converted to actionable serviceControlFailed guidance directing users to fresh status and the repair UI. No external exhaustive matches depend on the new internal KanataDaemonServiceError cases.

All required checks are green; proceeding to merge and exact-merge release acceptance.