Fix two service-lifecycle defects found during the physical-HID proof - #1300
Conversation
A failed `keypath service restart` could leave com.keypath.kanata absent from the launchd system domain entirely, with no command-line path back: `stop()` removes the SMAppService registration before it verifies its postcondition, and `start()` swallows a failed re-register. Proven on a disposable macOS 26 guest, where launchctl then reported no such service and `system repair` needed user action to recover. Restart now restores the registration before surfacing the failure, so a restart that cannot succeed leaves the caller no worse off than before it ran. A registration merely awaiting approval is left alone, since re-registering would discard the pending approval. Starting also polls briefly while a fresh registration settles. SMAppService registers asynchronously, so the status read taken immediately after register() can still report notRegistered and fail a start that would have worked moments later. That is the likely cause of recovery needing two attempts after an emergency stop. Also corrects two user-facing messages that sent this investigation down the wrong path: the restart failure hint blamed administrator authorization without mentioning the service state, and the runtime wizard said "Click Fix" while its buttons read Start and Restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
User-visible bug: with the built-in "Home Row Arrows" collection enabled
(it claims `f` on the base layer as a momentary layer activator), running
keypath-cli rule add f --tap f --hold lsft --on-conflict replace --apply
reported the rule as created. The rule could never take effect: config
generation keeps the collection's `layer_home-arrows_f` binding and drops
the custom `beh_base_f` one, and `ConfigurationService.generateConfiguration`
refuses to generate at all once `RuleCollectionDeduplicator.detectConflicts`
sees the collision (the activator-vs-mapping branch, #667).
Reproduced both halves in tests. The detector does fire, so the conflict is
not invisible — but it fires far too late. `RulesFacade.addRule` compared the
new rule only against other custom rules in CustomRulesStore, never against
collections, so it persisted the rule and reported success; the apply that
followed then failed, and the rule stayed in the store poisoning every later
`keypath config apply` until it was removed by hand. `--on-conflict replace`
could not help, because it cannot see a collection-owned key at all.
Fix, matching this codebase's detect-and-explain lineage (#458, #462, #463,
#464, #466, #667) and the GUI's interactive resolution (#460) rather than
silently letting one side win: the CLI-operation `RulesFacade` now loads
RuleCollections.json and pre-flights the prospective rule through the same
`detectConflicts` the generator uses, before anything is written. Conflicts
that already existed between collections are subtracted, so a store that is
already in a bad state does not block an unrelated key. `--on-conflict=skip`
no-ops; every other strategy raises `CLICollectionConflictError`, which
`rule add` renders as an exit-4 conflict naming the owning collection and
telling the user to disable it or pick another key. Nothing is persisted.
The app-facing `RulesFacade()` and `RulesFacade(store:)` initialisers supply
no collection loader, so their behaviour is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Reviewed the diff. One correctness concern worth a look, plus a minor consistency note.
let before = Set(... .map(\.description))
return after.filter { !before.contains($0.description) }This works only if Minor: Everything else — the |
|
CI note: Exit 75 is this project's infrastructure-busy signal, so this is a wait on runner disk rather than a rejection of the change. The runner's own |
Xcode 26.6 was upgraded in place on the build machine, leaving a bundle still named Xcode-26.6.app that actually contains Xcode 27.0 (27A266a). The pin guard compares the reported version, so it correctly refused the mismatch — and every canonical build, test, deploy, and release script stopped working, CI included. Nothing had built since, because the only runs in between were docs-only or a ping job. Advance the pin rather than reinstall the old toolchain: 26.6 no longer exists on the machine, and the bundle now holds the release build of 27. The bundle is renamed to Xcode-27.app so its name matches its contents; the misleading name is what made this look like a missing install. Verified per the documented procedure: ensure-metal-toolchain.sh passes with no override, DeploymentScriptContractTests passes through run-tests-safe.sh, and both suites from this branch run clean through the canonical runner rather than a bare swift test. Note for anyone bisecting: Xcode 27's Metal toolchain reports unavailable on the same invocation that downloads it, and resolves once the asset cryptex mounts. Re-run the script rather than concluding it failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
One behavioral inconsistency worth a look before merge:
The new-rule path correctly special-cases skip: let conflicts = await collectionConflicts(adding: rules, baseline: baseline)
if !conflicts.isEmpty {
if onConflict == .skip { return .skipped }
throw CLICollectionConflictError(...)
}But the merge path (updating an existing custom rule for the same rules[existingIndex] = merged
let conflicts = await collectionConflicts(adding: rules, baseline: baseline)
if !conflicts.isEmpty {
throw CLICollectionConflictError(input: input, conflicts: conflicts, ruleName: merged.displayTitle)
}So Minor/non-blocking: No other correctness/reliability issues found in the diff — the |
Advancing the pin exposed 40 warnings that Xcode 26.6 accepted silently. The build and test guardrails allow none, so nothing could merge until these were addressed. Neither group was introduced by this branch. Sendable conformance placement (24 warnings, all in one file). Six classes conform to wizard protocols that inherit Sendable, declared in WizardProtocolConformances.swift rather than beside each class. Swift 6 requires the conformance to live with its type, so each class now states it in its own file. The conformance was already in force through the protocol, so this records an existing guarantee where the compiler wants it rather than making a new claim about thread safety. A test subclass has to restate the inherited conformance, so it does. Environment access outside a View (16 warnings, two call sites). The wizard dismissal path read @Environment(\.dismiss), and the overlay keycap read @Environment(\.services), from tests that construct those views without installing them in a hierarchy. SwiftUI warns on every such read. Both views now accept an injected value that tests supply, and fall back to the environment in the app, where it is real. The overlay test even documented its reliance on the environment default; that reliance is now explicit rather than incidental. Verified with the canonical runner across all affected suites: 106 tests pass with zero build-log and test-log warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Automated review of this diff — a few points worth a second look:
No blocking correctness issues found in the |
Two loose ends from the Xcode 27 warning cleanup. The wizard status overview read @State from a test that never installs it in a view hierarchy, which SwiftUI warns about and which hands back a fresh instance on every read. It takes the same injected-value treatment as the other two views: tests supply the value, the app still uses real state. The formatter also has to be satisfied on every file this branch touches, not only on the lines it changed. An earlier attempt reverted that whole-file formatting to keep the diff tight, which was the wrong call: the code-quality gate lints entire changed files, so the reformatting is required rather than incidental churn. The Sendable notes are doc comments now, which is what the docComments rule wants ahead of a declaration. 129 tests pass across every affected suite with zero warnings, and swiftformat reports nothing left to format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the diff (note: the tool truncated the tail of Findings:
Looks solid:
|
1 participant
Two defects surfaced while running the P02 physical-HID proof on a disposable
macOS 26 guest. Both leave the user believing something worked when it did not.
A failed restart left the daemon unregistered
keypath service restartcould leavecom.keypath.kanataabsent from thelaunchd system domain entirely, with no command-line path back. On the guest,
launchctlthen reported no such service andkeypath system repairreturneduserActionRequired; only the app UI recovered it.Cause:
restart()isstop()thenstart(). Stop removes the SMAppServiceregistration before it verifies its postcondition, and start swallows a failed
re-register with
try?. Either path can throw with the registration alreadygone, and nothing rolls it back.
Restart now restores the registration before surfacing the failure, so a restart
that cannot succeed leaves the caller no worse off than before it ran. A
registration merely awaiting approval is left alone, since re-registering would
discard the pending approval.
Start also polls briefly while a fresh registration settles. SMAppService
registers asynchronously, so the status read taken immediately after
register()can still report
notRegisteredand fail a start that would have worked a momentlater. That is the likely cause of recovery needing two attempts after an
emergency stop, which is what we observed.
Also corrects two messages that actively misdirected this investigation: the
restart hint blamed administrator authorization without mentioning the service
state, and the runtime wizard said "Click Fix" while its buttons read Start and
Restart.
A rule for a collection-owned key was persisted, then refused
With the built-in Home Row Arrows collection enabled, which claims
fas amomentary layer activator:
reported the rule as created, and the apply that followed then refused. The rule
stayed in
CustomRules.jsonand broke every laterkeypath config applyuntilremoved by hand.
--on-conflict replacecould not help, becauseaddRulecompared the new rule only against other custom rules and never against
collections.
The conflict detector itself works correctly and fires as intended (#667); it
simply fires far too late, after the write. The facade now pre-flights the
prospective rule set through that same detector before anything is written, and
rule addrenders the refusal as an exit-4 conflict naming the owning collection.Conflicts already present between collections are subtracted, so a store in a bad
state does not block an unrelated key. The app-facing initializers supply no
collection loader, so GUI behavior is unchanged.
Verification
25 tests across the two areas, including 3 new daemon regression tests covering
rollback after a failed stop, rollback after a failed register, and the settling
poll; and 9 new tests covering the conflict refusal, the skip strategy, and the
cases that must still succeed. Adjacent rule, config-generation, deduplication,
and lifecycle suites all pass.
Note: the repo's pinned test scripts could not run here, because the bundle at
Xcode-26.6.appcurrently contains Xcode 27 and the pin guard rejects it. Testswere run with
swift testdirectly. That drift is worth a separate look.🤖 Generated with Claude Code