Complete catalog-led configuration boundaries - #1293
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Automated review of the supplied diff (note: diff was truncated after ~60KB, so ~40% of changed files — e.g. Findings from the visible portion:
Nothing else in the visible diff stood out as a clear correctness/security/perf bug; the added reproducibility guard ( |
|
Automated review of the supplied diff (note: diff was truncated after ~60000 bytes at 1. Likely journal-recovery regression for pre-existing version-2 journals (
let usesLegacyRoles = journal.version == 1 && journalRoles == scope.legacyRoles
guard journal.version == 1 || journal.version == 2,
(usesLegacyRoles || (journal.entries.count == files.count && journalRoles == Set(files.keys))),
...This only forgives 2. Possible permanent block on recovered-runtime-refresh ( guard needsRecoveredRuntimeRefresh, !needsRecoveredDeviceRuntimeRestart, let reloadHandler else { return nil }Once 3. Minor perf note (
No issues found with the |
![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: a2ca21d0a8
ℹ️ 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".
| let changed = hasFileActuallyChanged() | ||
| pendingAtomicWriteEvent = false |
There was a problem hiding this comment.
When an external editor saves keypath.kbd by rename/delete, rebindFileMonitor() calls setupFileMonitoring(), which records the replacement's fingerprint as the new baseline before the debounce fires. This makes hasFileActuallyChanged() return false here, and clearing pendingAtomicWriteEvent removes the previous forced-callback path, so common atomic saves never reach the hot-reload callback and the running configuration remains stale. Preserve the pre-rebind fingerprint or honor the atomic-write marker during reconciliation.
AGENTS.md reference: AGENTS.md:L342-L344
Useful? React with 👍 / 👎.
| public static func generateFromMappings(_ mappings: [KeyMapping]) -> String { | ||
| let collections = [RuleCollection].collection(named: "Custom Mappings", mappings: mappings) | ||
| return generateFromCollections(collections) | ||
| return generateFromCollections(collections, inputs: .empty) |
There was a problem hiding this comment.
When connected-device state has been populated, production callers such as ConfigurationManager.handleInvalidStartupConfig, ConfigurationService.backupFailedConfigAndApplySafe, and RuntimeCoordinator.resetToDefaultConfig still use this convenience API to write a live fallback. Passing .empty now silently removes the selected-device directives and VirtualHID exclusion that this API previously obtained from the cache; applying such a fallback can therefore remap disabled keyboards or let Kanata grab its VirtualHID output and enter a feedback loop. These callers need a captured DeviceGenerationInput rather than the empty compatibility input.
Useful? React with 👍 / 👎.
| let candidate = ShortcutListGenerationInput( | ||
| triggerMode: triggerMode ?? self.triggerMode, | ||
| holdDelayPreset: holdDelayPreset ?? self.holdDelayPreset, | ||
| customHoldDelayMs: customHoldDelayMs ?? self.customHoldDelayMs |
There was a problem hiding this comment.
When a user enters a custom delay below 100 ms or above 2000 ms, this candidate bypasses the existing PreferencesService clamp and is journaled into both defaults and the generated config unchanged. The later reload only clamps the in-memory preference, while this view restores the unbounded candidate and the persisted configuration remains out of range, so the UI, defaults, and runtime disagree. Normalize the candidate with the same 100–2000 ms bounds before starting the retained write.
Useful? React with 👍 / 👎.
| let proposedKeys = normalizedKeys(for: proposed) | ||
| let affectedKeys = existingKeys.symmetricDifference(proposedKeys).sorted() | ||
| let affectedLayers = Set([existing.targetLayer.displayName, proposed.targetLayer.displayName]).sorted() | ||
| let conflict = conflictInfo(for: proposed) |
There was a problem hiding this comment.
For a persisted collection that is disabled, conflictInfo(for:) still compares its proposed mappings against enabled collections because that helper does not inspect the candidate's isEnabled flag. Thus a disabled catalog entry that overlaps an active rule is marked canApply == false, even though updating it cannot affect the generated configuration; users cannot keep disabled built-ins current unless they first disturb unrelated active rules. Only perform this conflict check when the proposed collection is enabled.
Useful? React with 👍 / 👎.
|
Reviewed the diff (note: it was truncated after 60000 bytes, so this covers the Config/RuntimeCoordinator portion only). 1. Notification-driven config regeneration appears to be dropped without a visible replacement (RuntimeCoordinator.swift). The 2. 3. Wrong error case for a parameter-misuse guard (ConfigurationService.swift, Minor nit: |
|
Review note: the diff was truncated before I could see the rest of Correctness: In guard needsRecoveredDeviceRuntimeRestart else { return false }
guard await restartHandler() else {
throw KeyPathError.configuration(.loadFailed(reason: "Recovered device targeting could not be restarted"))
}
needsRecoveredDeviceRuntimeRestart = false
needsRecoveredRuntimeRefresh = false
return true
guard needsRecoveredRuntimeRefresh, !needsRecoveredDeviceRuntimeRestart, let reloadHandler else { return nil }So a single restart failure right after crash recovery (e.g. Kanata briefly unable to restart) permanently disables recovered-config reload for the rest of the process lifetime, with no retry path visible in the supplied diff. Worth adding either a retry on next save/reload attempt or surfacing this as a user-facing "needs restart" state rather than silently no-op'ing forever. Minor: |
|
Correctness: pre-existing v2 recovery journals will fail to recover after this upgrade (
let usesLegacyRoles = journal.version == 1 && journalRoles == scope.legacyRoles
guard journal.version == 1 || journal.version == 2,
usesLegacyRoles || (journal.entries.count == files.count && journalRoles == Set(files.keys)),The surrounding code already accepted Suggest widening the legacy check to Lower-confidence note (context was truncated in the diff for this file, so I can't fully verify): (Diff was truncated after 60000 bytes; review covers only the supplied portion.) |
|
Automated review (partial — the supplied diff was truncated after 60000 bytes, so files after Redundant generation-input reload on every rule write ( In let persistedInputs: GlobalRuleGenerationInputs? = if appSpecificKeys == nil || shortcutListGenerationInput == nil || deviceGenerationInput == nil {
try await persistedGlobalRuleGenerationInputs(preferenceDefaults: PreferencesService.canonicalDefaults)
} else {
nil
}Since Worth double-checking: watcher-suppression behavior removed (
ruleCollectionsManager.onBeforeSave = nil
No other actionable correctness/security issues found in the visible portion; the recovery-journal legacy-role handling and new device-targeting reproducibility checks in |
|
Reviewed the supplied diff (note: it was truncated after ~60KB, so files after 1. New reproducibility guard may block existing users' first save after upgrading ( 2. Duplicated UserDefaults key string literals in 3. Everything else in the visible portion (CI concurrency/mise pinning, |
|
Reviewed the supplied diff (note: it was truncated after ~60000 bytes, so this covers only the portion shown). Findings:
No blocking correctness issues found in the parts of the diff I could see beyond #1, which is worth a maintainer confirmation before merge. |
|
Reviewed the supplied diff (note: the diff was truncated after ~60000 bytes, so files after 1.
guard await restartHandler() else {
throw KeyPathError.configuration(.loadFailed(reason: "Recovered device targeting could not be restarted"))
}
needsRecoveredDeviceRuntimeRestart = false
needsRecoveredRuntimeRefresh = falseIf Meanwhile guard needsRecoveredRuntimeRefresh, !needsRecoveredDeviceRuntimeRestart, let reloadHandler else { return nil }So after one failed device-restart recovery, every subsequent normal rule-recovery reload becomes a silent no-op ( 2. Precondition violation reuses guard packRecord == nil || deviceSelections == nil else {
throw RecoverableRuleWrite.Failure.invalidJournal
}Everywhere else No other actionable correctness/security issues found in the reviewed portion; the added test coverage ( |
1 participant
Summary
Validation
Review