From 60be6b2d403ead1bdac5d116c14f47ee0fca8880 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Tue, 20 Oct 2026 05:29:55 -0700 Subject: [PATCH] Admission cleanup: remove dead conflict helpers, name media keys in verdicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delete what the engine made dead: resolvableCollectionConflict (resolved parties by name; the save path has resolved by id via mappingConflictContext since 4b) and its tests, conflictWithCustomRules (no production caller since 4a), the presenter's unused oneLine, and the startupFailureMessage shim that only delegated to SessionConfigurationRefusal. Presenter sentences name media keys and Caps Lock as the user knows them through the existing LayerMappingBuilder.mediaKeyDisplayLabel table. See docs/architecture/rules-admission-review.md §4.4h. Co-Authored-By: Claude Fable 5.1 --- .../AdmissionVerdictPresenter.swift | 18 ++-- ...CollectionsManager+ConflictDetection.swift | 16 ---- ...ollectionsManager+ConflictResolution.swift | 35 ------- .../SessionCapsRuntimeSupport.swift | 5 - .../MappingConflictResolutionDialog.swift | 4 +- .../AdmissionVerdictPresenterTests.swift | 4 +- ...RuleCollectionConflictDetectionTests.swift | 33 ------- .../SaveTimeConflictResolutionTests.swift | 96 ------------------- .../SessionConfigurationRefusalTests.swift | 11 +-- docs/architecture/rules-admission-review.md | 21 +++- 10 files changed, 36 insertions(+), 207 deletions(-) delete mode 100644 Tests/KeyPathTests/RuleCollections/SaveTimeConflictResolutionTests.swift diff --git a/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift b/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift index d3182d44e..2b6179640 100644 --- a/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift +++ b/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift @@ -30,8 +30,8 @@ enum AdmissionVerdictPresenter { let who = names.count == 2 ? "\(names[0]) and \(names[1])" : names.joined(separator: ", ") let what = details.isEmpty ? "" : " (\(details.joined(separator: "; ")))" return AdmissionPresentation( - title: "Key conflict on \(key)", - explanation: "\(who) both map \(key) on the \(layer) layer\(what). Only one can win.", + title: "Key conflict on \(keyLabel(key))", + explanation: "\(who) both map \(keyLabel(key)) on the \(layer) layer\(what). Only one can win.", options: verdict.fixes.map { option($0, name) } ) case let .unmetNeed(need): @@ -69,21 +69,21 @@ enum AdmissionVerdictPresenter { } } - /// For `automatic` verdicts: the one line worth showing afterwards. - static func oneLine(_ verdict: AdmissionVerdict, name: (AdmissionSubject) -> String) -> String { - let presentation = present(verdict, name: name) - return verdict.severity == .automatic ? presentation.explanation : presentation.title - } - static func describe(_ need: RuleEnvironmentNeed) -> String { switch need { - case let .emits(key): "sends \(key), which the current backend can't send" + case let .emits(key): "sends \(keyLabel(key)), which the current backend can't send" case .capsLockInput: "needs Caps Lock as an input" case .deviceTargeting: "targets specific keyboards, which isn't supported in driverless mode" case let .externalApp(app, _): "needs \(app) installed" } } + /// Media keys and Caps Lock by their names; everything else as Kanata says it. + static func keyLabel(_ kanataKey: String) -> String { + if kanataKey.lowercased() == "caps" { return "Caps Lock" } + return LayerMappingBuilder.mediaKeyDisplayLabel(kanataKey) ?? kanataKey + } + private static func displayName(_ subject: AdmissionSubject, _ name: (AdmissionSubject) -> String) -> String { if case let .named(text) = subject { return text } return name(subject) diff --git a/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift b/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift index de6504d86..127f85983 100644 --- a/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift +++ b/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift @@ -61,22 +61,6 @@ extension RuleCollectionsManager { ) } - /// Would mappings on `keys` (Kanata names) on `layer` collide with an enabled - /// custom rule? Kept for callers that have keys rather than a collection; - /// answered through the same engine via a synthetic candidate. - func conflictWithCustomRules(_ keys: Set, layer: RuleCollectionLayer) -> RuleConflictInfo? { - let probe = RuleCollection( - id: UUID(), name: "probe", summary: "", category: .custom, - mappings: keys.sorted().map { KeyMapping(input: $0, action: .keystroke(key: $0)) }, - isEnabled: true, icon: "", tags: [], targetLayer: layer, momentaryActivator: nil, configuration: .list - ) - let collections = ruleCollections.filter { !isNeovimAppScopedReference($0) } - let state = admissionState(collections: collections + [probe], customRules: customRules) - guard let info = firstConflict(involving: .collection(probe.id), in: state), - case .customRule = info.source else { return nil } - return info - } - /// The first conflict verdict naming `subject`, folded to one other party and /// all the keys they collide on. A synthetic party (the leader key, a chord /// group) is reported too; the dialog shows it without offering to disable it. diff --git a/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictResolution.swift b/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictResolution.swift index 8b6345bd0..b8e91c434 100644 --- a/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictResolution.swift +++ b/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictResolution.swift @@ -6,41 +6,6 @@ import KeyPathRulesCore extension RuleCollectionsManager { // MARK: - Save-time mapping-conflict resolution (#460) - /// Determine whether a set of save-time mapping conflicts can be resolved by - /// disabling a collection — i.e. every named party of every conflict maps to a - /// real, enabled collection by exact name. Synthetic parties (chord group names, - /// alias key-pairs, the "Leader Key" preference) won't match a collection, so - /// those conflicts fall back to a plain explanation rather than offering an - /// action that wouldn't apply. - /// - /// - Returns: the distinct collections the user could disable (≥2), or nil when - /// the conflicts aren't all collection-vs-collection. - nonisolated static func resolvableCollectionConflict( - conflicts: [KeyPathError.MappingConflictInfo], - collections: [RuleCollection] - ) -> [RuleCollection]? { - guard !conflicts.isEmpty else { return nil } - - let byName = Dictionary( - collections.filter(\.isEnabled).map { ($0.name, $0) }, - uniquingKeysWith: { first, _ in first } - ) - - var involved: [UUID: RuleCollection] = [:] - for conflict in conflicts { - // Every party must resolve to a real enabled collection; otherwise the - // whole set is treated as non-actionable (explanation fallback). - for name in conflict.conflictingCollections { - guard let collection = byName[name] else { return nil } - involved[collection.id] = collection - } - } - - // Need at least two distinct collections for "disable one" to resolve anything. - guard involved.count >= 2 else { return nil } - return involved.values.sorted { $0.name < $1.name } - } - /// On a save-time mapping-conflict failure, if the conflicting parties are all /// real collections, prompt the user to disable one and retry the save (#460). /// - Returns: the retry result, or nil when the failure wasn't a resolvable diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionCapsRuntimeSupport.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionCapsRuntimeSupport.swift index 6377bb893..17a83a038 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionCapsRuntimeSupport.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionCapsRuntimeSupport.swift @@ -290,11 +290,6 @@ enum SessionCapsRuntimeSupport { return selection } - /// Without attribution; the lifecycle coordinator adds the engine's. - static func startupFailureMessage(_ result: KanataHostBridgeValidationResult) -> String? { - SessionConfigurationRefusal.message(result) - } - static func journalDirectory(configPath: String = KeyPathConstants.Config.mainConfigPath) -> URL { URL(fileURLWithPath: configPath).deletingLastPathComponent().appendingPathComponent(".session-caps-mapping", isDirectory: true) } diff --git a/Sources/KeyPathAppKit/UI/Rules/MappingConflictResolutionDialog.swift b/Sources/KeyPathAppKit/UI/Rules/MappingConflictResolutionDialog.swift index 933dad6e5..cc63dd288 100644 --- a/Sources/KeyPathAppKit/UI/Rules/MappingConflictResolutionDialog.swift +++ b/Sources/KeyPathAppKit/UI/Rules/MappingConflictResolutionDialog.swift @@ -12,8 +12,8 @@ struct MappingConflictOption: Sendable, Equatable, Identifiable { /// Context for a save-time mapping conflict that can be resolved by disabling a /// collection. Sendable so it can cross the actor boundary from the save task to -/// the UI. Built only when every conflicting party is a real, toggleable collection -/// (see `RuleCollectionsManager.resolvableCollectionConflict`). +/// the UI. Built from the engine's verdicts by `RuleCollectionsManager.mappingConflictContext(for:)`, +/// only when every conflicting party is a real, toggleable collection. struct MappingConflictContext: Sendable, Equatable { /// Plain-English explanation of the conflict(s). let explanation: String diff --git a/Tests/KeyPathTests/RuleCollections/AdmissionVerdictPresenterTests.swift b/Tests/KeyPathTests/RuleCollections/AdmissionVerdictPresenterTests.swift index 53ea16523..d7113e97a 100644 --- a/Tests/KeyPathTests/RuleCollections/AdmissionVerdictPresenterTests.swift +++ b/Tests/KeyPathTests/RuleCollections/AdmissionVerdictPresenterTests.swift @@ -41,13 +41,13 @@ final class AdmissionVerdictPresenterTests: XCTestCase { reason: .unmetNeed(.emits(key: "volu")), fixes: [.disable(.collection(UUID()))]) let p = AdmissionVerdictPresenter.present(verdict, name: names) XCTAssertEqual(p.title, "Home Row Mods can't run here") - XCTAssertEqual(p.explanation, "Home Row Mods sends volu, which the current backend can't send.") + XCTAssertEqual(p.explanation, "Home Row Mods sends Volume Up, which the current backend can't send.") } func testReservedKeyMoveIsOneLineAndBlockedFormHasAnOption() { let move = AdmissionVerdict(severity: .automatic, subject: .customRule(UUID()), reason: .reservedKey(key: "f18"), fixes: [.moveCapsIntermediate(candidates: ["f19", "f20"])]) - XCTAssertEqual(AdmissionVerdictPresenter.oneLine(move, name: names), + XCTAssertEqual(AdmissionVerdictPresenter.present(move, name: names).explanation, "My rule uses F18, which KeyPath had reserved for Caps Lock. Caps Lock will use F19 instead.") let blocked = AdmissionVerdict(severity: .blocking, subject: .customRule(UUID()), reason: .reservedKey(key: "f18"), fixes: [.disable(.customRule(UUID()))]) diff --git a/Tests/KeyPathTests/RuleCollections/RuleCollectionConflictDetectionTests.swift b/Tests/KeyPathTests/RuleCollections/RuleCollectionConflictDetectionTests.swift index 03f32b15d..c6c08510b 100644 --- a/Tests/KeyPathTests/RuleCollections/RuleCollectionConflictDetectionTests.swift +++ b/Tests/KeyPathTests/RuleCollections/RuleCollectionConflictDetectionTests.swift @@ -207,39 +207,6 @@ final class RuleCollectionConflictDetectionTests: XCTestCase { XCTAssertNotNil(conflict) } - // MARK: - conflictWithCustomRules - - func testConflictWithCustomRules_FindsMatchingKey() { - let manager = makeManager() - var rule = CustomRule(input: "x", action: .keystroke(key: "y")) - rule.isEnabled = true - manager.customRules = [rule] - - let conflict = manager.conflictWithCustomRules(Set(["x"]), layer: .base) - XCTAssertNotNil(conflict) - } - - func testConflictWithCustomRules_NoMatchWhenDifferentLayer() { - let manager = makeManager() - var rule = CustomRule(input: "x", action: .keystroke(key: "y")) - rule.isEnabled = true - rule.targetLayer = .base - manager.customRules = [rule] - - let conflict = manager.conflictWithCustomRules(Set(["x"]), layer: .navigation) - XCTAssertNil(conflict) - } - - func testConflictWithCustomRules_NoMatchWhenDisabled() { - let manager = makeManager() - var rule = CustomRule(input: "x", action: .keystroke(key: "y")) - rule.isEnabled = false - manager.customRules = [rule] - - let conflict = manager.conflictWithCustomRules(Set(["x"]), layer: .base) - XCTAssertNil(conflict) - } - // MARK: - RuleConflictInfo func testRuleConflictInfo_DisplayName_Collection() { diff --git a/Tests/KeyPathTests/RuleCollections/SaveTimeConflictResolutionTests.swift b/Tests/KeyPathTests/RuleCollections/SaveTimeConflictResolutionTests.swift deleted file mode 100644 index 7c12b36ab..000000000 --- a/Tests/KeyPathTests/RuleCollections/SaveTimeConflictResolutionTests.swift +++ /dev/null @@ -1,96 +0,0 @@ -@testable import KeyPathAppKit -import KeyPathCore -import KeyPathRulesCore -import XCTest - -/// Tests for the save-time mapping-conflict resolvability core (#460): -/// a conflict is resolvable-by-disable only when every named party maps to a -/// real, enabled collection. -final class SaveTimeConflictResolutionTests: XCTestCase { - private func collection(_ name: String, isEnabled: Bool = true) -> RuleCollection { - RuleCollection( - name: name, - summary: name, - category: .productivity, - mappings: [KeyMapping(input: "a", action: .keystroke(key: "b"))], - isEnabled: isEnabled - ) - } - - private func conflict(_ collections: [String], key: String = ";") -> KeyPathError.MappingConflictInfo { - KeyPathError.MappingConflictInfo( - inputKey: key, - layer: "Base", - conflictingCollections: collections - ) - } - - func testResolvableWhenAllPartiesAreRealCollections() { - let collections = [collection("Home Row Mods"), collection("Home Row Layer Toggles")] - let conflicts = [conflict(["Home Row Mods", "Home Row Layer Toggles"])] - - let resolvable = RuleCollectionsManager.resolvableCollectionConflict( - conflicts: conflicts, collections: collections - ) - - XCTAssertEqual(resolvable?.map(\.name), ["Home Row Layer Toggles", "Home Row Mods"]) - } - - func testNotResolvableWhenAPartyIsSynthetic() { - // Chord-group-name conflicts name "chord group '...'", which is not a collection. - let collections = [collection("Chord Groups")] - let conflicts = [conflict(["chord group 'navigation'", "chord group 'navigation'"], key: "navigation")] - - XCTAssertNil(RuleCollectionsManager.resolvableCollectionConflict( - conflicts: conflicts, collections: collections - )) - } - - func testNotResolvableWhenLeaderKeyPartyHasNoCollection() { - // The leader conflict names "Leader Key" (a preference, not a collection). - let collections = [collection("Window Mgmt")] - let conflicts = [conflict(["Leader Key", "Window Mgmt"], key: "spc")] - - XCTAssertNil(RuleCollectionsManager.resolvableCollectionConflict( - conflicts: conflicts, collections: collections - )) - } - - func testNotResolvableWhenPartyCollectionIsDisabled() { - // A disabled collection isn't in the active set, so it can't be matched. - let collections = [collection("Home Row Mods"), collection("Home Row Layer Toggles", isEnabled: false)] - let conflicts = [conflict(["Home Row Mods", "Home Row Layer Toggles"])] - - XCTAssertNil(RuleCollectionsManager.resolvableCollectionConflict( - conflicts: conflicts, collections: collections - )) - } - - func testNotResolvableWhenOnlyOneDistinctCollection() { - let collections = [collection("Home Row Mods")] - let conflicts = [conflict(["Home Row Mods", "Home Row Mods"])] - - XCTAssertNil(RuleCollectionsManager.resolvableCollectionConflict( - conflicts: conflicts, collections: collections - )) - } - - func testMixedResolvableAndSyntheticFallsBack() { - // If any conflict in the set is non-actionable, the whole set falls back. - let collections = [collection("Home Row Mods"), collection("Home Row Layer Toggles")] - let conflicts = [ - conflict(["Home Row Mods", "Home Row Layer Toggles"]), - conflict(["caps-lock / caps_lock"], key: "caps-lock / caps_lock") - ] - - XCTAssertNil(RuleCollectionsManager.resolvableCollectionConflict( - conflicts: conflicts, collections: collections - )) - } - - func testEmptyConflictsNotResolvable() { - XCTAssertNil(RuleCollectionsManager.resolvableCollectionConflict( - conflicts: [], collections: [collection("X")] - )) - } -} diff --git a/Tests/KeyPathTests/RuleCollections/SessionConfigurationRefusalTests.swift b/Tests/KeyPathTests/RuleCollections/SessionConfigurationRefusalTests.swift index d0c9cfbff..da6322a4f 100644 --- a/Tests/KeyPathTests/RuleCollections/SessionConfigurationRefusalTests.swift +++ b/Tests/KeyPathTests/RuleCollections/SessionConfigurationRefusalTests.swift @@ -43,16 +43,15 @@ final class SessionConfigurationRefusalTests: KeyPathTestCase { let message = SessionConfigurationRefusal.message(.invalid(reason: "unsupported output key volu")) XCTAssertEqual(message, "This configuration cannot run in the driverless session: unsupported output key volu. " + "Edit the configuration and retry; existing rules were preserved.") - XCTAssertEqual(message, SessionCapsRuntimeSupport.startupFailureMessage(.invalid(reason: "unsupported output key volu"))) } func testInvalidWithAttributionNamesTheRuleFirst() { let message = SessionConfigurationRefusal.message( .invalid(reason: "unsupported output key volu"), - attribution: ["Louder sends volu, which the current backend can't send."] + attribution: ["Louder sends Volume Up, which the current backend can't send."] ) XCTAssertEqual(message, "This configuration cannot run in the driverless session. " - + "Louder sends volu, which the current backend can't send. " + + "Louder sends Volume Up, which the current backend can't send. " + "Turn that rule off or change it, then retry; existing rules were preserved. " + "(Validator: unsupported output key volu.)") } @@ -70,7 +69,7 @@ final class SessionConfigurationRefusalTests: KeyPathTestCase { manager.customRules = [CustomRule(title: "Louder", input: "f12", action: .keystroke(key: "volu")), CustomRule(title: "Fine", input: "f11", action: .keystroke(key: "a"))] XCTAssertEqual(manager.blockingAdmissionSentences(profile: noMedia), - ["Louder sends volu, which the current backend can't send."]) + ["Louder sends Volume Up, which the current backend can't send."]) XCTAssertEqual(manager.blockingAdmissionSentences(profile: allowAll), []) } @@ -94,7 +93,7 @@ final class SessionConfigurationRefusalTests: KeyPathTestCase { let findings = manager.admissionFindings(profile: noMedia) XCTAssertEqual(findings.map(\.severity), ["blocking", "needsChoice", "automatic"]) XCTAssertEqual(findings[0].subject, "Louder") - XCTAssertEqual(findings[0].explanation, "Louder sends volu, which the current backend can't send.") + XCTAssertEqual(findings[0].explanation, "Louder sends Volume Up, which the current backend can't send.") XCTAssertEqual(findings[0].fixes, ["Turn off Louder"]) XCTAssertEqual(findings[1].title, "Key conflict on f13") XCTAssertEqual(Set(findings[1].fixes), ["Turn off A", "Turn off B"]) @@ -105,7 +104,7 @@ final class SessionConfigurationRefusalTests: KeyPathTestCase { func testFindingsRoundTripThroughTheControlChannel() throws { let finding = RuntimeControl.AdmissionFinding(severity: "blocking", subject: "Louder", title: "Louder can't run here", - explanation: "Louder sends volu, which the current backend can't send.", + explanation: "Louder sends Volume Up, which the current backend can't send.", fixes: ["Turn off Louder"]) var response = RuntimeControl.Response(ok: true, operation: .doctor, state: .failed, workerPID: nil, failure: "x", lastError: nil, layer: nil, appPID: 1, appVersion: "1") diff --git a/docs/architecture/rules-admission-review.md b/docs/architecture/rules-admission-review.md index 24dd912f3..f8f6ff56d 100644 --- a/docs/architecture/rules-admission-review.md +++ b/docs/architecture/rules-admission-review.md @@ -354,7 +354,7 @@ Callers, all through this one function: ### 4.4c Phase 4b (2026-10-19) - `AdmissionVerdictPresenter` words every verdict once (title, explanation, options - from its fixes; `oneLine` for automatic verdicts). Dialogs, toasts and the CLI + from its fixes). Dialogs, toasts and the CLI render from it. - The save-time dialog (`MappingConflictContext`) is now built by `RuleCollectionsManager.mappingConflictContext(for:)` from the engine's conflict @@ -433,8 +433,7 @@ Callers, all through this one function: ### 4.4f Phase 4e (2026-10-19) - **One renderer.** `SessionConfigurationRefusal.message(result, attribution:)` is the - only place "this configuration cannot run" is worded. `startupFailureMessage` delegates - to it; the lifecycle coordinator (`admitSessionConfiguration`'s error, + only place "this configuration cannot run" is worded; the lifecycle coordinator (`admitSessionConfiguration`'s error, `sessionConfigurationRefusal`, `currentRuntimeStatus`) and the save gate (`requireSessionEligibleConfiguration`) call it, so the setup screen, Status, the reload error and a refused save all say the same sentence. @@ -487,6 +486,22 @@ The detector's test corpus (`RuleCollectionDeduplicatorTests`, `MultiCollectionC unchanged, which is the proof the replacement preserves behaviour; two tests were added for what it fixes (same-titled rules told apart; details carried). +### 4.4h Cleanup (2026-10-20) + +Dead paths removed once the engine was the only implementation: +`RuleCollectionsManager.resolvableCollectionConflict` (resolved parties *by name*; the +save path has used `mappingConflictContext(for:)`, by id, since 4b) with its tests; +`conflictWithCustomRules` (no production caller since 4a); the presenter's `oneLine`; +and the `startupFailureMessage` shim that only delegated to `SessionConfigurationRefusal`. +Presenter sentences now name media keys and Caps Lock as the user knows them +("sends Volume Up", "Key conflict on Caps Lock") through the one existing table, +`LayerMappingBuilder.mediaKeyDisplayLabel`; other keys keep their Kanata names. + +`KeyPathError.MappingConflictInfo` stays, deliberately: it is the save gate's error, +carries names and details rather than ids, and lives in KeyPathCore where the verdict +types cannot be seen. It is produced from verdicts in exactly one place +(`AdmissionEngine.mappingConflicts`). + ### 4.5 Lowering: intent in rules, backend output in the generator Rules keep expressing intent (`volu`, `caps` → Hyper, "this keyboard only").