[malpern]

Caps Lock rules currently pause when more than one keyboard is connected, even though ordinary mappings apply globally. This change gives each eligible keyboard its own crash-recoverable Caps Lock mapping, applies the same rules on all of them by default, and provides per-keyboard exclusions in General settings. An existing saved single-keyboard choice keeps its scope on upgrade; Settings offers an explicit switch to all keyboards. The cramped Connected Keyboard card and contradictory Caps status are also fixed.

Validation: the bridge-backed full Swift suite passed after the main review fixes; focused fleet and migration tests passed after the final status refinement; Swift build and accessibility checks passed. An opt-in live test on the mini mapped Caps on all three attached USB keyboard services (cl80, X2A, and the KeyPath physical HID fixture), read each mapping back, restored the originals, and verified each restore. The settings and Vim image snapshots still differ from their existing references (the same two mismatches were observed before this change).

Review: local Claude CLI review could not complete because its OAuth session expired. GitHub claude-review found scope migration, picker, recovery, retry, and warning issues, which were addressed in follow-up commits. The worker isolates a device failure when its original mapping can be restored; an unknown owner or failed restore still stops the session because the mapping state cannot be trusted.

Remaining qualification: hot-plug timing has not been measured. The worker checks arrivals every two seconds, so an immediate Caps press after plugging in can precede mapping. Keep this PR unmerged until the final checks pass and that limitation is accepted.

[claude]

Review of PR #1339 (the diff was truncated after 60 KB, so the rest of SettingsView.swift, the tests and the docs were not reviewed). Findings:

1. Silent consent change on upgrade (SessionCapsRuntimeSupport.swift, ServiceLifecycleCoordinator+SessionRuntime.swift). selectedKeyboardOnlyKey defaults to false, so existing users who consented to one exact keyboard now get hidutil mappings on every keyboard. Their saved selection is ignored. This contradicts the "exact-device Caps consent" contract in RuntimeCoordinator. Consider migrating users who have a saved selection to selected-only mode, or asking for consent again.

2. Selected-only mode can't be entered or changed from the UI (SessionCapsSettingsSection.swift). The keyboard Picker was removed. setSelectedOnly(true) and choose(_:) have no remaining caller in the visible diff. In selected-only mode the only control is "Use on all keyboards". After choosingOneKeyboard = true the per-keyboard toggles call setKeyboardEnabled, not choose. If choose really has no caller, the advanced mode is dead code and users who are already in it are stuck.

3. Legacy recovery failure blocks fleet recovery (SessionCapsRuntimeSupport.recoverPending). recoverLegacyPending runs first and throws, so SessionCapsMappingFleet.recover never runs. A stale or foreign legacy record then leaves per-keyboard mappings unrestored. Run both recoveries and rethrow the first error, as fleet.recover does internally.

4. Failed devices are retried too often (checkCapsFleetIfNeeded). reconcile calls acquire again every 2 seconds for each uncovered device. Each attempt runs hidutil and writes an intent journal, and the failure is only logged once. A keyboard with a foreign Caps mapping will cause this write churn indefinitely. Add a per-device backoff or only retry when the inventory changes.

5. Worker exits on any non-busy reconcile error. A single device's restore or verify failure calls finish(.failed, ownershipLost). That stops Caps handling for all keyboards, including ones that are working. This conflicts with the "failed device left uncovered" design stated in acquire's comment. Consider limiting the exit to errors that mean ownership was actually lost.

6. Settings polling (SessionCapsSettingsSection). A 3-second timer runs reviewKeyboard, which spawns hidutil, enumerates devices and verifies leases, whenever the section is visible and rules use Caps. That is a lot of subprocess work for a status UI. A longer interval, or refreshing on the HID arrival notification, would be cheaper.

7. Test coverage. SessionCapsMappingFleetTests covers the fleet type only. Nothing visible covers the worker's reconcile and activation transitions (previous.isEmpty → activate), the migration behaviour in item 1, or capsCoverageWarning.

[claude]

Review of the supplied diff. The diff was truncated at 60KB, so SettingsView.swift and anything after it were not reviewed.

Findings

1. Silent scope migration (correctness/safety). selectedKeyboardOnlyKey defaults to false. Existing users with a saved single-keyboard Caps selection will now have Caps remapped on every keyboard at next start, with no migration or consent. Consider defaulting to selected-only when selectionKey data exists. Without a migration, users who deliberately chose one keyboard get a behavior change.
2. Worker dies on transient HID errors (reliability). In checkCapsFleetIfNeeded, any non-busy failure, for example a hidutil enumeration hiccup in SessionCapsHIDUtilTransport.keyboards(), calls finish(.failed, ownershipLostReason). That tears down the whole runtime, including non-Caps rules. Consider tolerating a few consecutive failures, and fail only on ownership or config mismatch.
3. Settings polling cost (performance). The 3s Timer in SessionCapsSettingsSection runs reviewKeyboard(). Each pass spawns hidutil, reads fleet journals and calls capsConflict per uncovered keyboard. Ticks that land while busy set refreshAgain, which can chain extra refreshes. Gate the timer on window visibility or use the HID arrival notification, and drop the tick when busy instead of queueing it.
4. Input state is never deactivated (minor). The fleet check activates capsInput when previous.isEmpty && !current.isEmpty. It never deactivates when all keyboards disconnect (current.isEmpty). Confirm that stale state is harmless.
5. Misplaced doc comment (nit). In ServiceLifecycleCoordinator+SessionRuntime.swift, chooseAllCapsIntermediate was inserted between chooseCapsSelection's doc comment and its declaration. The comment now documents the wrong function.
6. Early return (clarify). The new selectedKeyboardOnlyKey condition in the guard at ServiceLifecycleCoordinator.swift:~430 skips that path entirely in the default all-keyboards mode. Please confirm that is intended.

Tests: SessionCapsMappingFleetTests covers the fleet. The new coordinator branches are untested: all-keyboards intermediate selection, capsCoverageWarning, and the exclusion/stop-restart path in setCapsExcludedKeyboardIDs.

[claude]

Review (diff truncated at 60KB; SettingsView, KanataViewModel, SessionRuntimeReport, tests and docs were not reviewed)

Findings:

1. Dead/mis-ordered code in ServiceLifecycleCoordinator+SessionRuntime.swift: the doc comment for chooseCapsSelection ("mapped, a keyboard shortcut, or a new rule since the last start…") now sits above the new chooseAllCapsIntermediate(), so it documents the wrong function. Move chooseAllCapsIntermediate above that comment.

2. Coverage warning overwrites the paused message (+SessionRuntime.swift, ~L144): if let coverage = report.capsCoverage { sessionCapsPaused = Self.capsCoverageWarning(coverage) } runs unconditionally after the paused message is set. When coverage is fully healthy it returns nil and clears a legitimate "could not start on a connected keyboard" warning. Guard it so it only applies in all-keyboards mode, and only replaces the message when the result is non-nil.

3. Fleet reconcile can fail the whole worker on one bad device (SessionRuntimeWorker.checkCapsFleetIfNeeded): reconcile throws (not just records a failure) when a pending record's owner differs, or when lease.restore fails for a vanished device. Any throw other than .busy or an inventory failure goes to finish(.failed, ownershipLostReason), which takes down every keyboard's mapping. This is probably intended fail-closed behavior, but it conflicts with the "one failed device stays uncovered" design in acquire. Please confirm and add a test.

4. Config-change check is skipped when the digest read fails: guard digest == nil || digest == admittedDigest treats an unreadable config as unchanged. The single-keyboard path may do the same. If so it's consistent, but a transient nil read delays detecting a swapped profile.

5. Exclusion changes may not take effect on a live worker: setCapsExcludedKeyboardIDs stops and restores the runtime, then resumes it. The worker re-reads excludedKeyboardIDs() only on launch and at each 2s check. It's fine if the stop/resume always happens, but if wasRunning is false, the preference is saved with no verification beyond the commit closure. Low risk.

6. Minor: preferenceID returns "" if encoding fails. An empty ID could match nothing, or collide across devices in the exclusion set. Prefer a throwing path or a fallback that keeps IDs distinct. reservedKey calls UInt32(exactly: UserDefaults.integer(...)), which returns 0 when unset and gives label(0) rather than the default usage. Handle 0 explicitly.

7. Test coverage: SessionCapsMappingFleetTests is added (252 lines). Per the visible diff, there's nothing for capsCoverageWarning, the upgrade-path default in selectedKeyboardOnly(), or the worker's reconcile failure paths. Consider adding tests for these.

No security issues found in the reviewed portion; the root-lock guard and directory ownership/mode checks look sound.

[claude]

Review of PR #1339. The diff was truncated at 60KB, so the Settings view, KanataViewModel, SessionRuntimeReport, tests and docs were not reviewed.

No blocking bugs found in the reviewed portion. Points worth checking:

1. Transient hidutil failures can take down the whole worker. In SessionRuntimeWorker.checkCapsFleetIfNeeded, three consecutive inventory failures (about 6s at the 2s cadence) call finish(.failed, ownershipLostReason). That stops all rules, not just Caps. A slow or hung hidutil or IORegistry during sleep/wake or hub churn could trigger it. Consider degrading to "Caps paused, revoke capsInput" and retrying, or using a longer or time-based threshold. capsFleetReadFailures is reset only on success, so failures that aren't consecutive in time still accumulate if a success is skipped by the busy early return.

2. Any non-busy fleet error is fatal. Any reconcile throw other than .busy (for example a wrongOwner or unsafeFile refusal from one stale device directory) calls finish(.failed). recordsOnDisk() throws on one bad entry under keyboards/, and deviceIDsOnDisk() throws on any non-numeric name, such as a stray .DS_Store. One junk file therefore makes Caps rules fail closed and also kills the worker. Skipping or quarantining unrecognised entries, or at least logging per device, would be safer.

3. Backoff only applies to new devices. capsFleetRetryAfter skips a device only if it was not in previous. A device that failed verification inside reconcile is restored and added to failures. On the next tick it is still in previous, so it is retried every 2s with no backoff, and the same lease acquire/restore cycle repeats. Clear it from previous when it fails, or apply the backoff unconditionally.

4. Possible stale coverage in the Settings UI. activeDeviceIDs filters on processAlive(workerPID) and verifyActive per record, so fleetUncovered can briefly show every keyboard as uncovered while the worker is restarting. The staggered 3s re-reviews help, but a flash of orange warnings on every start is likely. Gating on sessionWantsRunning or runtime state would avoid it.

5. Untested paths. SessionCapsMappingFleetTests covers the fleet, but I didn't see tests for these in the visible portion:

   • the capsFleetReadFailures threshold,
   • selectedKeyboardOnly() upgrade-default semantics,
   • chooseAllCapsIntermediate (setup-off and invalid paths),
   • capsCoverageWarning wording.

   The upgrade default (existing selection means keep selected-only) is the riskiest silent behaviour change and deserves a unit test.

6. Minor. The doc comment that began "mapped, a keyboard shortcut, or a new rule since the last start…" for chooseCapsSelection is now attached to chooseAllCapsIntermediate. Move it back to the right function.

[claude]

Review (diff truncated at 60 KB, so the rest of SessionCapsSettingsSection, SettingsView*, KanataViewModel, SessionRuntimeReport, the tests and the docs were not reviewed.)

I found no blocking correctness bugs in the reviewed portion. The fleet lease design looks sound: root termination guard, per-device recovery that keeps the first failure, and fail-closed restore. Notes:

1. Polling cost, SessionRuntimeWorker.checkCapsFleetIfNeeded. Every 2s, for the life of the worker, this runs SessionCapsHIDUtilTransport.keyboards() (hidutil). reconcile then calls verifyActive per leased device, which probably does more hidutil reads. The old single-device path was 0.5s but verified one device. Consider re-enumerating only on an HID device add/remove notification and using a slower heartbeat otherwise. Alternatively, confirm the per-tick hidutil cost is acceptable on machines with several keyboards.

2. Zero-keyboard startup. In all-keyboards mode with no eligible keyboard, capsRecord is nil, so the report's managedCapsGeneration is nil. The coordinator's liveness path handles this with the "waiting for a compatible keyboard" warning. The older intended && managedCapsGeneration != nonce helper (near the end of +SessionRuntime) may still treat it as a failed Caps start. Please check that no restart or paused-state loop results, and add a test for the empty-fleet start.

3. Misplaced doc comment. In ServiceLifecycleCoordinator+SessionRuntime.swift, the existing doc block about the intermediate reservation and "nil when the profile does not need managed Caps" now sits above chooseAllCapsIntermediate. It was written for chooseCapsSelection. The new advanced-path comment sits below it. Move the original block back.

4. Test coverage. SessionCapsMappingFleetTests covers the fleet. I saw no tests for capsCoverageWarning, the setCapsExcludedKeyboardIDs bounds (128 IDs, 4096 bytes), the selectedKeyboardOnly() upgrade default, or the worker reconcile/retry (30s backoff, allowNewDevices while keys are held). The first three are cheap pure-function tests.

5. Minor. excludedKeyboardIDs filters by byte length on read but not by count. This is harmless but inconsistent with the 128 cap on write.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-24T14:46:30.503479Z	ecb0852	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ecb0852a03

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

Release admitted Caps input before retiring its lease

If reconciliation retires the last keyboard lease while Caps Lock is held, this revokes admission without releasing logical usage 57 from Kanata or clearing its held output. This can happen after a transient verification read failure: reconcile restores the keyboard's original mapping and returns no records, so the eventual physical release no longer arrives as the reserved intermediate and cannot release the admitted Caps press. A Caps-to-Control rule can therefore leave Control stuck while the worker continues running. Release the admitted runtime input and reconcile its output ledger before revoking or replacing coverage, or use the existing shutdown cleanup path.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Suppress the legacy conflict warning for verified fleet mappings

With one connected keyboard in the new default all-keyboards mode, the fleet can be fully active yet Settings still tells the user that another app is remapping Caps Lock and must be turned off. This new branch correctly excludes active devices from uncovered, but refreshKeyboards subsequently still calls Self.conflict(on: device, intermediate: saved?.intermediateUsage). All-keyboards mode has no saved selection, so the intermediate is nil and ownRow cannot recognize KeyPath's own mapping. Skip that legacy single-device conflict check in fleet mode or use the verified fleet ownership when suppressing its own row.

Useful? React with 👍 / 👎.