From ba5578f729c2906cf2245fe05124fdfb7340ec22 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Tue, 20 Oct 2026 05:15:24 -0700 Subject: [PATCH] Admission engine owns conflict detection: claims by rule id replace the wrapped detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 wrapped RuleCollectionDeduplicator.detectConflicts, which reported display names, and mapped names back to ids — so two rules titled alike were both blamed for one collision. The engine now computes every conflict from claims (what each owner puts on which slot): key slots with the leader key, activator-vs-mapping, activator-vs-activator, chord groups within an owner, and alias-name collisions. Reason.conflict carries the details (hold action, activator target, alias origin) the presenter and the save gate's MappingConflictInfo both render. The old detector is deleted; the save gate and the CLI call AdmissionEngine.mappingConflicts, and the detector's test corpus runs against the engine unchanged. See docs/architecture/rules-admission-review.md §4.4g. Co-Authored-By: Claude Fable 5.1 --- Sources/KeyPathAppKit/CLI/RulesFacade.swift | 8 +- .../Config/ConfigurationService.swift | 10 +- .../KanataConfiguration+BlockBuilders.swift | 2 +- .../Models/RuleCollectionDeduplicator.swift | 261 +----------------- .../Services/Packs/PackInstaller.swift | 2 +- .../RuleCollections/AdmissionEngine.swift | 240 ++++++++++++++-- .../AdmissionVerdictPresenter.swift | 5 +- ...CollectionsManager+ConflictDetection.swift | 2 +- .../KeyPathRulesCore/AdmissionVerdict.swift | 10 +- .../CLI/CLIRuleCollectionConflictTests.swift | 6 +- .../AliasDeduplicationTests.swift | 4 +- .../ChordGroupsIntegrationTests.swift | 4 +- .../AdmissionEngineTests.swift | 39 ++- .../AdmissionToggleParityTests.swift | 2 +- .../DeduplicatorEdgeCaseTests.swift | 4 +- .../MultiCollectionConflictTests.swift | 6 +- .../RuleCollectionDeduplicatorTests.swift | 54 ++-- ...onfigurationServiceSavePipelineTests.swift | 6 +- docs/architecture/rules-admission-review.md | 29 ++ 19 files changed, 349 insertions(+), 345 deletions(-) diff --git a/Sources/KeyPathAppKit/CLI/RulesFacade.swift b/Sources/KeyPathAppKit/CLI/RulesFacade.swift index e6d4faea5..cba42ff13 100644 --- a/Sources/KeyPathAppKit/CLI/RulesFacade.swift +++ b/Sources/KeyPathAppKit/CLI/RulesFacade.swift @@ -53,14 +53,10 @@ public struct RulesFacade: Sendable { guard let collectionLoader else { return [] } let collections = await collectionLoader() // Custom rules first, exactly as generateConfiguration orders them. - let after = RuleCollectionDeduplicator.detectConflicts( - in: rules.asRuleCollections() + collections - ) + let after = AdmissionEngine.mappingConflicts(in: rules.asRuleCollections() + collections) guard !after.isEmpty else { return [] } let before = Set( - RuleCollectionDeduplicator - .detectConflicts(in: baselineRules.asRuleCollections() + collections) - .map(\.description) + AdmissionEngine.mappingConflicts(in: baselineRules.asRuleCollections() + collections).map(\.description) ) return after.filter { !before.contains($0.description) } } diff --git a/Sources/KeyPathAppKit/Infrastructure/Config/ConfigurationService.swift b/Sources/KeyPathAppKit/Infrastructure/Config/ConfigurationService.swift index 47e95cade..771285df0 100644 --- a/Sources/KeyPathAppKit/Infrastructure/Config/ConfigurationService.swift +++ b/Sources/KeyPathAppKit/Infrastructure/Config/ConfigurationService.swift @@ -1111,13 +1111,9 @@ public final class ConfigurationService: FileConfigurationProviding { let leaderKeyPref = leaderKeyPreference ?? storedLeaderKeyPref let shortcutInput = shortcutListGenerationInput - // DETECT CONFLICTS BEFORE DEDUPLICATION - // This catches cases where multiple collections map the same key, and where - // a collection activator collides with the system leader key (#463). - let conflicts = RuleCollectionDeduplicator.detectConflicts( - in: collectionsForConfig, - leaderKey: leaderKeyPref - ) + // Admission before deduplication: two intentional claims on one slot are + // refused here, so `dedupe` below never resolves one in silence (ADR-039 §1). + let conflicts = AdmissionEngine.mappingConflicts(in: collectionsForConfig, leaderKey: leaderKeyPref) if !conflicts.isEmpty { AppLogger.shared.log( "⚠️ [ConfigService] Mapping conflicts detected: \(conflicts.map(\.description).joined(separator: "; "))" diff --git a/Sources/KeyPathAppKit/Infrastructure/Config/KanataConfiguration+BlockBuilders.swift b/Sources/KeyPathAppKit/Infrastructure/Config/KanataConfiguration+BlockBuilders.swift index 34ade7223..29b03357e 100644 --- a/Sources/KeyPathAppKit/Infrastructure/Config/KanataConfiguration+BlockBuilders.swift +++ b/Sources/KeyPathAppKit/Infrastructure/Config/KanataConfiguration+BlockBuilders.swift @@ -518,7 +518,7 @@ extension KanataConfiguration { /// Safety net: detect duplicate alias names and keep only the last definition /// so kanata doesn't reject the config outright. /// - /// The primary defense is `RuleCollectionDeduplicator.detectConflicts()` which + /// The primary defense is `AdmissionEngine.conflicts(in:leaderKey:)()` which /// blocks config writes when collections overlap. This function catches edge /// cases that slip past (e.g., aliases generated from non-mapping paths). /// It logs warnings so the root cause is visible in diagnostics. diff --git a/Sources/KeyPathAppKit/Models/RuleCollectionDeduplicator.swift b/Sources/KeyPathAppKit/Models/RuleCollectionDeduplicator.swift index 7a19439cd..668786318 100644 --- a/Sources/KeyPathAppKit/Models/RuleCollectionDeduplicator.swift +++ b/Sources/KeyPathAppKit/Models/RuleCollectionDeduplicator.swift @@ -3,258 +3,9 @@ import KeyPathCore import KeyPathRulesCore enum RuleCollectionDeduplicator { - // MARK: - Conflict Detection - - /// Detects mapping conflicts BEFORE deduplication. - /// Returns conflicts where multiple collections map the same key in the same layer. - /// Call this before `dedupe()` to catch conflicts that would otherwise be silently resolved. - /// - /// Uses `effectiveMappings(for:)` to include config-generated mappings (Home Row Mods, - /// Layer Toggles, etc.) — not just the raw `.mappings` array. - static func detectConflicts( - in collections: [RuleCollection], - leaderKey: LeaderKeyPreference? = nil - ) -> [KeyPathError.MappingConflictInfo] { - struct ClaimInfo { - let collectionName: String - let holdDescription: String? - } - - var claimedKeys: [InputKey: [ClaimInfo]] = [:] - // Which collections place a regular mapping on each (input, layer) slot, - // keyed by collection id → name. Mappings only (NOT activators or the leader) - // so it can be cross-checked against activator placements without - // double-reporting. Keyed by id, not name: collection titles aren't unique - // (custom rules become collections titled by editable display text), so a - // name-based self-filter could cancel the real other owner. - var mappingOwners: [InputKey: [UUID: String]] = [:] - - for collection in collections where collection.isEnabled { - let mappings = KanataConfiguration.effectiveMappings(for: collection) - var seenKeysInCollection = Set() - for mapping in mappings { - let normalizedInput = KanataKeyConverter.convertToKanataKey(mapping.input) - // Skip duplicate keys within the same collection (e.g., chord groups - // sharing keys). Within-collection conflicts are detected separately. - guard seenKeysInCollection.insert(normalizedInput).inserted else { continue } - let inputKey = InputKey(input: normalizedInput, layer: collection.targetLayer) - mappingOwners[inputKey, default: [:]][collection.id] = collection.name - - let holdDesc = mapping.behavior.flatMap { behavior -> String? in - if case let .dualRole(dr) = behavior { - return dr.holdActionString - } - return nil - } - - claimedKeys[inputKey, default: []].append( - ClaimInfo(collectionName: collection.name, holdDescription: holdDesc) - ) - } - } - - // The leader key (#463) also consumes its physical key's BASE-layer slot: - // buildCollectionBlocks emits the leader as a base entry (tap = key, hold = - // layer) before any collection, and deduplicateBlocks keeps it over a later - // base mapping — so a base-layer collection or custom rule that maps the same - // key is silently dropped. Claim that base slot so the collision surfaces. - if let leaderKey, leaderKey.enabled { - let normalizedInput = KanataKeyConverter.convertToKanataKey(leaderKey.key) - let inputKey = InputKey(input: normalizedInput, layer: .base) - claimedKeys[inputKey, default: []].append( - ClaimInfo( - collectionName: "Leader Key", - holdDescription: "\(leaderKey.targetLayer.displayName) layer" - ) - ) - } - - var conflicts: [KeyPathError.MappingConflictInfo] = [] - for (inputKey, claims) in claimedKeys where claims.count > 1 { - let collectionNames = claims.map(\.collectionName) - let holdDescriptions = claims.compactMap { claim -> String? in - guard let hold = claim.holdDescription else { return nil } - return "\(claim.collectionName): hold → \(hold)" - } - - conflicts.append(KeyPathError.MappingConflictInfo( - inputKey: inputKey.input, - layer: inputKey.layer.displayName, - conflictingCollections: collectionNames, - holdDescriptions: holdDescriptions - )) - } - - // Momentary activator conflicts (#466): two collections claim the same - // physical activator key from the same source layer but route it to - // different target layers. The silent `seenActivators` dedup in - // buildCollectionBlocks would drop one without notice — surface it via the - // same detect-and-explain path instead. - // - // The claim slot is (sourceLayer, input): a key can only carry one binding - // per layer. The same key activating different targets from *different* - // source layers is a valid chained-layer setup (e.g. `f` → arrows from base - // and `f` → function from nav), not a conflict. Identical activators (same - // slot + same target) are redundant, not conflicting. Both are skipped so - // the detection does not over-fire (matching the toggle-time conflictInfo). - struct ActivatorSlot: Hashable { - let sourceLayer: RuleCollectionLayer - let input: String - } - struct ActivatorClaim { - let collectionName: String - let targetLayer: RuleCollectionLayer - } - var activatorClaims: [ActivatorSlot: [ActivatorClaim]] = [:] - // The system leader key (#463) is an activator too: pressing it from the - // base layer activates its target layer. buildCollectionBlocks seeds it into - // seenActivators before any collection, so a collection activator on the - // same base-layer key is silently dropped — include it as a claim so the - // collision is surfaced instead. - if let leaderKey, leaderKey.enabled { - let slot = ActivatorSlot( - sourceLayer: .base, - input: KanataKeyConverter.convertToKanataKey(leaderKey.key) - ) - activatorClaims[slot, default: []].append( - ActivatorClaim(collectionName: "Leader Key", targetLayer: leaderKey.targetLayer) - ) - } - for collection in collections where collection.isEnabled { - guard let activator = collection.momentaryActivator else { continue } - // "hyper" activators are folded into the hyper hold action, not emitted - // as standalone layer activators — they never collide in seenActivators. - guard activator.input.lowercased() != "hyper" else { continue } - let normalizedInput = KanataKeyConverter.convertToKanataKey(activator.input) - let slot = ActivatorSlot(sourceLayer: activator.sourceLayer, input: normalizedInput) - activatorClaims[slot, default: []].append( - ActivatorClaim(collectionName: collection.name, targetLayer: activator.targetLayer) - ) - - // Activator-vs-mapping conflict: this activator occupies its key's slot - // on its source layer (buildCollectionBlocks places the tap-hold there), - // but another collection maps the same key on that layer. The generator - // emits both and silently keeps one (e.g. Home Row Arrows' `f` activator - // shadowing Home Row Mods' hold-`f`). Surface it. Activator-vs-activator - // is handled by the redundant-aware pass below; leader collisions via the - // leader injection above — neither is in `mappingOwners`, so no double-report. - let mappingSlot = InputKey(input: normalizedInput, layer: activator.sourceLayer) - // Exclude the activator's own collection by id (names aren't unique). - let otherMappers = (mappingOwners[mappingSlot] ?? [:]) - .filter { $0.key != collection.id } - .values.sorted() - if !otherMappers.isEmpty { - conflicts.append(KeyPathError.MappingConflictInfo( - inputKey: normalizedInput, - layer: activator.sourceLayer.displayName, - conflictingCollections: [collection.name] + otherMappers, - holdDescriptions: ["\(collection.name): activates \(activator.targetLayer.displayName)"] - + otherMappers.map { "\($0): maps \(normalizedInput)" } - )) - } - } - for (slot, claims) in activatorClaims where claims.count > 1 { - // Only a conflict if the activators disagree on which layer to activate. - let distinctTargets = Set(claims.map(\.targetLayer)) - guard distinctTargets.count > 1 else { continue } - conflicts.append(KeyPathError.MappingConflictInfo( - inputKey: slot.input, - layer: slot.sourceLayer.displayName, - conflictingCollections: claims.map(\.collectionName), - holdDescriptions: claims.map { "\($0.collectionName): activates \($0.targetLayer.displayName)" } - )) - } - - // Within-collection chord group conflicts: two chord groups sharing keys - // produces duplicate aliases that kanata rejects. - for collection in collections where collection.isEnabled { - if case let .chordGroups(config) = collection.configuration { - // Duplicate group names (#464): each group renders to a - // `(defchords …)` block keyed by its name, so two groups - // sharing a name produce duplicate blocks that kanata rejects. - for duplicate in config.detectDuplicateGroupNames() { - conflicts.append(KeyPathError.MappingConflictInfo( - inputKey: duplicate.name, - layer: collection.targetLayer.displayName, - conflictingCollections: Array( - repeating: "chord group '\(duplicate.name)'", - count: duplicate.count - ), - holdDescriptions: [ - "\(duplicate.count) chord groups share the name '\(duplicate.name)' — chord group names must be unique" - ] - )) - } - - let chordConflicts = config.detectCrossGroupConflicts() - for conflict in chordConflicts { - let groupNames = conflict.groups.map(\.name) - let chordDescriptions = conflict.groups.map { group -> String in - let chordOutputs = group.chords - .filter { $0.keys.contains(conflict.key) } - .map { "(\($0.keys.joined(separator: "+")) → \($0.action.displayName))" } - .joined(separator: ", ") - return "\(group.name): \(chordOutputs)" - } - conflicts.append(KeyPathError.MappingConflictInfo( - inputKey: conflict.key, - layer: collection.targetLayer.displayName, - conflictingCollections: groupNames, - holdDescriptions: chordDescriptions - )) - } - } - } - - // Alias-name collisions (#462): generated alias names are - // `{prefix}_{layer}_{sanitized-input}`, and sanitization maps `-`/space (and, - // for device switches, any non-alphanumeric) to `_`. Two distinct input keys - // can therefore collapse to the same alias name (e.g. `caps-lock` and - // `caps_lock` → `beh_base_caps_lock`), which kanata rejects. These keys don't - // collide as defsrc inputs, so the key-overlap pass above misses them. - struct AliasOrigin { - let collectionName: String - let rawInput: String - let layer: String - } - var aliasOrigins: [String: [AliasOrigin]] = [:] - for collection in collections where collection.isEnabled { - for mapping in KanataConfiguration.effectiveMappings(for: collection) { - // Chord inputs (space-separated) render to defchordsv2, not aliases. - guard !mapping.input.contains(" ") else { continue } - for aliasName in KanataConfiguration.generatedAliasNames( - for: mapping, layer: collection.targetLayer - ) { - aliasOrigins[aliasName, default: []].append(AliasOrigin( - collectionName: collection.name, - rawInput: mapping.input, - layer: collection.targetLayer.displayName - )) - } - } - } - for (aliasName, origins) in aliasOrigins { - // Only a collision when *different* input keys produce the same alias. - // Same input across collections is already a key-overlap conflict above. - let distinctInputs = Set(origins.map(\.rawInput)) - guard distinctInputs.count > 1 else { continue } - conflicts.append(KeyPathError.MappingConflictInfo( - inputKey: distinctInputs.sorted().joined(separator: " / "), - layer: origins.first?.layer ?? RuleCollectionLayer.base.displayName, - conflictingCollections: Array(Set(origins.map(\.collectionName))).sorted(), - holdDescriptions: ["keys \(distinctInputs.sorted().joined(separator: ", ")) all generate the alias '\(aliasName)'"] - )) - } - - // Total ordering (inputKey, then layer, then explanation) so output is - // deterministic even when two conflicts share an inputKey — e.g. the same - // pair of keys colliding on two different alias prefixes at once. - return conflicts.sorted { lhs, rhs in - if lhs.inputKey != rhs.inputKey { return lhs.inputKey < rhs.inputKey } - if lhs.layer != rhs.layer { return lhs.layer < rhs.layer } - return lhs.holdDescriptions.joined() < rhs.holdDescriptions.joined() - } - } + // Conflict detection lives in `AdmissionEngine.conflicts(in:leaderKey:)`, + // attributed by rule id. This type only applies the generator's structural + // "first claim wins" after admission has refused every intentional overlap. /// Deduplicates collections by removing duplicate input keys. /// - First collection to claim an input key (per layer) wins @@ -309,9 +60,9 @@ enum RuleCollectionDeduplicator { let inputKey = InputKey(input: normalizedInput, layer: collection.targetLayer) if seenInputKeys.contains(inputKey) { - // Admission (detectConflicts / AdmissionEngine) runs before this and - // refuses any two intentional claims on one slot, so reaching here is - // a gap in detection, not a resolution: never silently (ADR-039 §1). + // AdmissionEngine.conflicts runs before this and refuses any two + // intentional claims on one slot, so reaching here is a gap in + // detection, not a resolution: never silently (ADR-039 §1). AppLogger.shared.warnUnlessQuietTest( "⚠️ [Dedup] Dropped mapping '\(mapping.input)' from '\(collection.name)' — key already claimed on \(collection.targetLayer.displayName) layer. Admission should have refused this." ) diff --git a/Sources/KeyPathAppKit/Services/Packs/PackInstaller.swift b/Sources/KeyPathAppKit/Services/Packs/PackInstaller.swift index 689731cf0..d5875f05c 100644 --- a/Sources/KeyPathAppKit/Services/Packs/PackInstaller.swift +++ b/Sources/KeyPathAppKit/Services/Packs/PackInstaller.swift @@ -738,7 +738,7 @@ public final class PackInstaller { let name = RuleCollectionsManager.nameLookup(state) let verdicts = AdmissionEngine.evaluate(state, profile: profile, fileExists: fileExists).filter { verdict in if involvesPack(verdict.subject) { return true } - if case let .conflict(_, _, parties) = verdict.reason { return parties.contains(where: involvesPack) } + if case let .conflict(_, _, parties, _) = verdict.reason { return parties.contains(where: involvesPack) } if case let .packConflict(other, _) = verdict.reason { return other == pack.id } return false } diff --git a/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionEngine.swift b/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionEngine.swift index b79acafd3..fe99c294f 100644 --- a/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionEngine.swift +++ b/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionEngine.swift @@ -6,11 +6,10 @@ import KeyPathRulesCore /// not: key conflicts, needs the backend or Mac cannot meet, the reserved Caps /// Lock intermediate key, and pack-level conflicts and requirements. /// -/// Phase 3 of docs/architecture/rules-admission-review.md. Pure and additive: -/// nothing calls it yet (phase 4 switches the toggle, editor, pack install, -/// save and startup paths to it). Conflicts come from the existing -/// `RuleCollectionDeduplicator.detectConflicts`, so they match today's results -/// exactly; this adds attribution to rule ids and the cases nothing detected. +/// Phase 3 of docs/architecture/rules-admission-review.md; every caller (toggle, +/// editor, pack install, save gate, startup, doctor) asks it. Conflicts are +/// computed here from claims — what each rule puts on which slot — attributed +/// by rule id, never by display name (§4.4g). @MainActor enum AdmissionEngine { struct State { @@ -81,30 +80,227 @@ enum AdmissionEngine { } } - // MARK: - Key conflicts (today's detector, attributed) + // MARK: - Key conflicts (claims, attributed by id) + + /// One party's stake in a slot: who, what they are called, and what the key + /// does for them (shown when two stakes collide). + private struct Stake { + let subject: AdmissionSubject + let name: String + let detail: String? + } + + private struct KeySlot: Hashable { + let input: String + let layer: RuleCollectionLayer + } + + /// Every conflict in `collections` (enabled ones; custom rules already folded + /// in as collections) plus the leader key. Pure, so the save gate and the CLI + /// call it off the main actor. Order is the slot order of the claims; callers + /// that need a total order sort (see `mappingConflicts`). + nonisolated static func conflicts(in collections: [RuleCollection], leaderKey: LeaderKeyPreference? = nil) + -> [AdmissionVerdict] + { + let owners: [(subject: AdmissionSubject, collection: RuleCollection)] = + collections.filter(\.isEnabled).map { (.collection($0.id), $0) } + return conflicts(owners: owners, leaderKey: leaderKey) + } + + /// The same, for the save gate and the CLI, in the error type they report. + nonisolated static func mappingConflicts(in collections: [RuleCollection], leaderKey: LeaderKeyPreference? = nil) + -> [KeyPathError.MappingConflictInfo] + { + let names = Dictionary(collections.map { ($0.id, $0.name) }, uniquingKeysWith: { first, _ in first }) + return conflicts(in: collections, leaderKey: leaderKey).compactMap { verdict in + guard case let .conflict(key, layer, parties, details) = verdict.reason else { return nil } + return KeyPathError.MappingConflictInfo( + inputKey: key, layer: layer, + conflictingCollections: parties.map { subject in + switch subject { + case let .collection(id), let .customRule(id): names[id] ?? "a rule" + case let .pack(id): id + case let .named(name): name + } + }, + holdDescriptions: details + ) + } + // Total order (key, layer, details) so the gate's message is deterministic + // even when two conflicts share a key (two alias prefixes at once). + .sorted { lhs, rhs in + if lhs.inputKey != rhs.inputKey { return lhs.inputKey < rhs.inputKey } + if lhs.layer != rhs.layer { return lhs.layer < rhs.layer } + return lhs.holdDescriptions.joined() < rhs.holdDescriptions.joined() + } + } private static func conflictVerdicts(_ state: State, owners: [(subject: AdmissionSubject, collection: RuleCollection)]) -> [AdmissionVerdict] { - let infos = RuleCollectionDeduplicator.detectConflicts( - in: state.collections + state.customRules.map { $0.asRuleCollection() }, - leaderKey: state.leaderKey - ) - return infos.map { info in - // The detector reports display names, which are not unique; map each - // back to every enabled owner with that name, or keep the name. - let parties: [AdmissionSubject] = info.conflictingCollections.flatMap { name in - let matches = owners.filter { $0.collection.name == name }.map(\.subject) - return matches.isEmpty ? [.named(name)] : matches - } - return AdmissionVerdict( + conflicts(owners: owners, leaderKey: state.leaderKey) + } + + private nonisolated static func conflicts(owners: [(subject: AdmissionSubject, collection: RuleCollection)], + leaderKey: LeaderKeyPreference?) -> [AdmissionVerdict] + { + var verdicts: [AdmissionVerdict] = [] + func conflict(_ key: String, _ layer: String, _ stakes: [Stake], subject: AdmissionSubject? = nil) { + verdicts.append(AdmissionVerdict( severity: .needsChoice, - subject: parties.first ?? .named(info.inputKey), - reason: .conflict(key: info.inputKey, layer: info.layer, parties: parties), - fixes: parties.map { .disable($0) } - ) + subject: subject ?? stakes[0].subject, + reason: .conflict(key: key, layer: layer, parties: stakes.map(\.subject), + details: stakes.compactMap(\.detail)), + fixes: stakes.map(\.subject).filter { if case .named = $0 { false } else { true } }.map { .disable($0) } + )) } + let leader = leaderKey.flatMap { $0.enabled ? $0 : nil } + let leaderInput = leader.map { KanataKeyConverter.convertToKanataKey($0.key) } + + // 1. Mappings: one (input, layer) slot carries one action. The leader key + // (#463) holds its physical key's base slot too: the generator emits it + // first and keeps it over a later base mapping, which would otherwise be + // dropped in silence. Within one owner a repeated input is not a + // conflict (chord groups share keys); that is checked in 4. + var keySlots: [KeySlot: [Stake]] = [:] + var slotOrder: [KeySlot] = [] + // Who maps each slot, by id, for the activator check in 2. + var mappers: [KeySlot: [(subject: AdmissionSubject, name: String)]] = [:] + func claim(_ slot: KeySlot, _ stake: Stake) { + if keySlots[slot] == nil { slotOrder.append(slot) } + keySlots[slot, default: []].append(stake) + } + for owner in owners { + var seen = Set() + for mapping in KanataConfiguration.effectiveMappings(for: owner.collection) { + let input = KanataKeyConverter.convertToKanataKey(mapping.input) + guard seen.insert(input).inserted else { continue } + let slot = KeySlot(input: input, layer: owner.collection.targetLayer) + var hold: String? + if case let .dualRole(dualRole)? = mapping.behavior { hold = dualRole.holdActionString } + claim(slot, Stake(subject: owner.subject, name: owner.collection.name, + detail: hold.map { "\(owner.collection.name): hold → \($0)" })) + mappers[slot, default: []].append((owner.subject, owner.collection.name)) + } + } + if let leader, let leaderInput { + claim(KeySlot(input: leaderInput, layer: .base), + Stake(subject: .named("Leader Key"), name: "Leader Key", + detail: "Leader Key: hold → \(leader.targetLayer.displayName) layer")) + } + for slot in slotOrder { + let stakes = keySlots[slot]! + guard stakes.count > 1 else { continue } + conflict(slot.input, slot.layer.displayName, stakes) + } + + // 2. An activator occupies its key on its source layer (the generator puts + // the tap-hold there), so another owner mapping that same slot loses + // silently (#466: Home Row Arrows' `f` shadowing Home Row Mods' hold-`f`). + // 3. Two activators on one (source layer, key) must agree on the target + // layer; the same key reaching different layers from different source + // layers is a valid chain, and identical activators are merely redundant. + struct ActivatorSlot: Hashable { + let sourceLayer: RuleCollectionLayer + let input: String + } + var activators: [ActivatorSlot: [(stake: Stake, target: RuleCollectionLayer)]] = [:] + var activatorOrder: [ActivatorSlot] = [] + func claimActivator(_ slot: ActivatorSlot, _ stake: Stake, target: RuleCollectionLayer) { + if activators[slot] == nil { activatorOrder.append(slot) } + activators[slot, default: []].append((stake, target)) + } + if let leader, let leaderInput { + claimActivator(ActivatorSlot(sourceLayer: .base, input: leaderInput), + Stake(subject: .named("Leader Key"), name: "Leader Key", + detail: "Leader Key: activates \(leader.targetLayer.displayName)"), + target: leader.targetLayer) + } + for owner in owners { + guard let activator = owner.collection.momentaryActivator, + activator.input.lowercased() != "hyper" // folded into the hyper hold, never a standalone activator + else { continue } + let input = KanataKeyConverter.convertToKanataKey(activator.input) + let name = owner.collection.name + claimActivator(ActivatorSlot(sourceLayer: activator.sourceLayer, input: input), + Stake(subject: owner.subject, name: name, + detail: "\(name): activates \(activator.targetLayer.displayName)"), + target: activator.targetLayer) + let others = (mappers[KeySlot(input: input, layer: activator.sourceLayer)] ?? []) + .filter { $0.subject != owner.subject } + .sorted { $0.name < $1.name } + guard !others.isEmpty else { continue } + conflict(input, activator.sourceLayer.displayName, + [Stake(subject: owner.subject, name: name, detail: "\(name): activates \(activator.targetLayer.displayName)")] + + others.map { Stake(subject: $0.subject, name: $0.name, detail: "\($0.name): maps \(input)") }) + } + for slot in activatorOrder { + let claims = activators[slot]! + guard claims.count > 1, Set(claims.map(\.target)).count > 1 else { continue } + conflict(slot.input, slot.sourceLayer.displayName, claims.map(\.stake)) + } + + // 4. Chord groups within one owner: two groups with one name render to two + // `(defchords …)` blocks (#464), and two groups sharing a key make + // duplicate aliases; kanata rejects both. + for owner in owners { + guard case let .chordGroups(config) = owner.collection.configuration else { continue } + let layer = owner.collection.targetLayer.displayName + for duplicate in config.detectDuplicateGroupNames() { + let group = Stake(subject: .named("chord group '\(duplicate.name)'"), + name: "chord group '\(duplicate.name)'", detail: nil) + var stakes = Array(repeating: group, count: duplicate.count) + stakes[0] = Stake(subject: group.subject, name: group.name, + detail: "\(duplicate.count) chord groups share the name '\(duplicate.name)' — chord group names must be unique") + conflict(duplicate.name, layer, stakes, subject: owner.subject) + } + for crossing in config.detectCrossGroupConflicts() { + conflict(crossing.key, layer, crossing.groups.map { group in + let chords = group.chords.filter { $0.keys.contains(crossing.key) } + .map { "(\($0.keys.joined(separator: "+")) → \($0.action.displayName))" } + .joined(separator: ", ") + return Stake(subject: .named(group.name), name: group.name, detail: "\(group.name): \(chords)") + }, subject: owner.subject) + } + } + + // 5. Alias names (#462): `{prefix}_{layer}_{sanitized input}` maps `-`, space + // (and more, for device switches) to `_`, so distinct inputs such as + // `caps-lock` and `caps_lock` can render to one alias, which kanata + // rejects. They never collide as inputs, so 1 does not see them. + struct AliasOrigin { + let subject: AdmissionSubject + let name: String + let input: String + let layer: String + } + var aliases: [String: [AliasOrigin]] = [:] + var aliasOrder: [String] = [] + for owner in owners { + for mapping in KanataConfiguration.effectiveMappings(for: owner.collection) + where !mapping.input.contains(" ") // chords render to defchordsv2, not aliases + { + for alias in KanataConfiguration.generatedAliasNames(for: mapping, layer: owner.collection.targetLayer) { + if aliases[alias] == nil { aliasOrder.append(alias) } + aliases[alias, default: []].append(AliasOrigin(subject: owner.subject, name: owner.collection.name, + input: mapping.input, + layer: owner.collection.targetLayer.displayName)) + } + } + } + for alias in aliasOrder { + let origins = aliases[alias]! + let inputs = Set(origins.map(\.input)).sorted() + guard inputs.count > 1 else { continue } // same input twice is a slot conflict, reported in 1 + var seen = Set() + let parties = origins.filter { seen.insert($0.subject).inserted }.sorted { $0.name < $1.name } + var stakes = parties.map { Stake(subject: $0.subject, name: $0.name, detail: nil) } + stakes[0] = Stake(subject: stakes[0].subject, name: stakes[0].name, + detail: "keys \(inputs.joined(separator: ", ")) all generate the alias '\(alias)'") + conflict(inputs.joined(separator: " / "), origins[0].layer, stakes) + } + return verdicts } // MARK: - Environment needs (phase 2 inference) diff --git a/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift b/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift index 296f13cb7..d3182d44e 100644 --- a/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift +++ b/Sources/KeyPathAppKit/Services/RuleCollections/AdmissionVerdictPresenter.swift @@ -25,12 +25,13 @@ enum AdmissionVerdictPresenter { static func present(_ verdict: AdmissionVerdict, name: (AdmissionSubject) -> String) -> AdmissionPresentation { let subject = displayName(verdict.subject, name) switch verdict.reason { - case let .conflict(key, layer, parties): + case let .conflict(key, layer, parties, details): let names = parties.map { displayName($0, name) } 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. Only one can win.", + explanation: "\(who) both map \(key) on the \(layer) layer\(what). Only one can win.", options: verdict.fixes.map { option($0, name) } ) case let .unmetNeed(need): diff --git a/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift b/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift index 44199ab30..de6504d86 100644 --- a/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift +++ b/Sources/KeyPathAppKit/Services/RuleCollections/RuleCollectionsManager+ConflictDetection.swift @@ -84,7 +84,7 @@ extension RuleCollectionsManager { var keysByParty: [AdmissionSubject: Set] = [:] var order: [AdmissionSubject] = [] for verdict in AdmissionEngine.evaluate(state) { - guard case let .conflict(key, _, parties) = verdict.reason, parties.contains(subject) else { continue } + guard case let .conflict(key, _, parties, _) = verdict.reason, parties.contains(subject) else { continue } for party in parties where party != subject { if keysByParty[party] == nil { order.append(party) } keysByParty[party, default: []].insert(key) diff --git a/Sources/KeyPathRulesCore/AdmissionVerdict.swift b/Sources/KeyPathRulesCore/AdmissionVerdict.swift index c9b27e8c3..b3cdd3d31 100644 --- a/Sources/KeyPathRulesCore/AdmissionVerdict.swift +++ b/Sources/KeyPathRulesCore/AdmissionVerdict.swift @@ -5,8 +5,8 @@ public enum AdmissionSubject: Hashable, Sendable { case collection(UUID) case customRule(UUID) case pack(String) - /// A party the detector names but no stored rule owns (leader key, a chord - /// group, an alias pair). Carries the detector's display name. + /// A party no stored rule owns (the leader key, a chord group inside a + /// collection). Carries its display name. case named(String) } @@ -32,8 +32,10 @@ public struct AdmissionVerdict: Hashable, Sendable { } public enum Reason: Hashable, Sendable { - /// Two or more parties claim `key` on `layer`. - case conflict(key: String, layer: String, parties: [AdmissionSubject]) + /// Two or more parties claim `key` on `layer`. `details` says what the + /// key does for each, in the parties' order, where that differs from a + /// plain mapping ("Home Row Mods: hold → lsft", "Arrows: activates nav"). + case conflict(key: String, layer: String, parties: [AdmissionSubject], details: [String] = []) /// A need the backend or Mac cannot meet. case unmetNeed(RuleEnvironmentNeed) /// The rule uses the key reserved as the Caps Lock intermediate. diff --git a/Tests/KeyPathTests/CLI/CLIRuleCollectionConflictTests.swift b/Tests/KeyPathTests/CLI/CLIRuleCollectionConflictTests.swift index 55d3b2850..3cd90d209 100644 --- a/Tests/KeyPathTests/CLI/CLIRuleCollectionConflictTests.swift +++ b/Tests/KeyPathTests/CLI/CLIRuleCollectionConflictTests.swift @@ -11,7 +11,7 @@ import XCTest /// `beh_base_f` binding never took effect. /// /// Two separate facts, both verified here: -/// 1. `RuleCollectionDeduplicator.detectConflicts` *does* see this collision +/// 1. `AdmissionEngine.conflicts` *does* see this collision /// (activator-vs-mapping, #667), so config generation refuses to build. /// 2. `RulesFacade.addRule` used to persist the rule anyway, because it only /// ever compared against other custom rules — so the rule was reported as @@ -61,7 +61,7 @@ final class CLIRuleCollectionConflictTests: XCTestCase { input: "f", action: tapHoldF().action, behavior: tapHoldF().behavior ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [rule].asRuleCollections() + [arrows] ) @@ -217,7 +217,7 @@ final class CLIRuleCollectionConflictTests: XCTestCase { defer { try? FileManager.default.removeItem(at: directory) } XCTAssertFalse( - RuleCollectionDeduplicator.detectConflicts(in: [arrows, conflicting]).isEmpty, + AdmissionEngine.mappingConflicts(in: [arrows, conflicting]).isEmpty, "Precondition: the collections conflict with each other" ) diff --git a/Tests/KeyPathTests/Infrastructure/AliasDeduplicationTests.swift b/Tests/KeyPathTests/Infrastructure/AliasDeduplicationTests.swift index b1adc8d90..42d6113e5 100644 --- a/Tests/KeyPathTests/Infrastructure/AliasDeduplicationTests.swift +++ b/Tests/KeyPathTests/Infrastructure/AliasDeduplicationTests.swift @@ -72,7 +72,7 @@ final class AliasDeduplicationTests: XCTestCase { )) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [ + let conflicts = AdmissionEngine.mappingConflicts(in: [ hrmCollection, layerTogglesCollection, ]) @@ -104,7 +104,7 @@ final class AliasDeduplicationTests: XCTestCase { )) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [ + let conflicts = AdmissionEngine.mappingConflicts(in: [ hrmCollection, layerTogglesCollection, ]) diff --git a/Tests/KeyPathTests/Infrastructure/ChordGroupsIntegrationTests.swift b/Tests/KeyPathTests/Infrastructure/ChordGroupsIntegrationTests.swift index d93b82cbe..e8bcf959e 100644 --- a/Tests/KeyPathTests/Infrastructure/ChordGroupsIntegrationTests.swift +++ b/Tests/KeyPathTests/Infrastructure/ChordGroupsIntegrationTests.swift @@ -431,7 +431,7 @@ final class ChordGroupsIntegrationTests: XCTestCase { ) // Conflict detection should catch shared keys between chord groups - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) XCTAssertFalse(conflicts.isEmpty, "Should detect conflicts when chord groups share keys") XCTAssertEqual(conflicts.count, 2, "Should detect 2 conflicting keys (s and d)") @@ -467,7 +467,7 @@ final class ChordGroupsIntegrationTests: XCTestCase { configuration: .chordGroups(config) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) XCTAssertTrue(conflicts.isEmpty, "No conflicts when chord groups use distinct keys") } } diff --git a/Tests/KeyPathTests/RuleCollections/AdmissionEngineTests.swift b/Tests/KeyPathTests/RuleCollections/AdmissionEngineTests.swift index a80101040..125c2c87c 100644 --- a/Tests/KeyPathTests/RuleCollections/AdmissionEngineTests.swift +++ b/Tests/KeyPathTests/RuleCollections/AdmissionEngineTests.swift @@ -33,11 +33,12 @@ final class AdmissionEngineTests: XCTestCase { private func assertConflictsMatchDetector(_ state: AdmissionEngine.State, file: StaticString = #filePath, line: UInt = #line) -> Int { - let detector = RuleCollectionDeduplicator.detectConflicts( + // The save gate's view of the same state must agree with evaluate's. + let detector = AdmissionEngine.mappingConflicts( in: state.collections + state.customRules.map { $0.asRuleCollection() }, leaderKey: state.leaderKey ) let conflicts = evaluate(state).compactMap { verdict -> (String, String)? in - guard case let .conflict(key, layer, _) = verdict.reason else { return nil } + guard case let .conflict(key, layer, _, _) = verdict.reason else { return nil } XCTAssertEqual(verdict.severity, .needsChoice, file: file, line: line) return (key, layer) } @@ -73,12 +74,44 @@ final class AdmissionEngineTests: XCTestCase { let a = collection("A", [("a", "1")]), b = collection("B", [("a", "2")]) let rule = CustomRule(title: "Mine", input: "a", action: .keystroke(key: "3")) let verdict = try XCTUnwrap(evaluate(.init(collections: [a, b], customRules: [rule])).first) - guard case let .conflict(key, _, parties) = verdict.reason else { return XCTFail("not a conflict") } + guard case let .conflict(key, _, parties, _) = verdict.reason else { return XCTFail("not a conflict") } XCTAssertEqual(key, "a") XCTAssertEqual(Set(parties), [.collection(a.id), .collection(b.id), .customRule(rule.id)]) XCTAssertEqual(Set(verdict.fixes), Set(parties.map { AdmissionVerdict.Fix.disable($0) })) } + /// Names are not identities: two rules titled alike, one colliding, must + /// blame only the one that collides (the old detector reported names). + func testSameTitledRulesAreToldApart() { + let colliding = CustomRule(title: "Mine", input: "a", action: .keystroke(key: "2")) + let innocent = CustomRule(title: "Mine", input: "b", action: .keystroke(key: "3")) + let a = collection("A", [("a", "1")]) + let verdicts = evaluate(.init(collections: [a], customRules: [colliding, innocent])) + XCTAssertEqual(verdicts.count, 1) + guard case let .conflict(_, _, parties, _)? = verdicts.first?.reason else { return XCTFail("not a conflict") } + XCTAssertEqual(Set(parties), [.collection(a.id), .customRule(colliding.id)]) + XCTAssertFalse(parties.contains(.customRule(innocent.id))) + // The save gate's error names them too, from the same verdicts. + let gate = AdmissionEngine.mappingConflicts(in: [a] + [colliding, innocent].map { $0.asRuleCollection() }) + XCTAssertEqual(gate.map(\.conflictingCollections), [["A", "Mine"]]) + } + + /// What each party does with the key travels with the verdict. + func testConflictDetailsNameTheHoldAndTheActivator() throws { + let mods = collection("Mods", [("f", "f")]) + var withHold = mods + withHold.mappings = [KeyMapping(input: "f", action: .keystroke(key: "f"), + behavior: .dualRole(DualRoleBehavior(tapAction: .keystroke(key: "f"), holdAction: .keystroke(key: "lsft"))))] + let arrows = collection("Arrows", [], activator: MomentaryActivator(input: "f", targetLayer: .navigation)) + let verdict = try XCTUnwrap(evaluate(.init(collections: [withHold, arrows])).first) + guard case let .conflict(key, layer, parties, details) = verdict.reason else { return XCTFail("not a conflict") } + XCTAssertEqual(key, "f"); XCTAssertEqual(layer, "Base") + XCTAssertEqual(parties, [.collection(arrows.id), .collection(withHold.id)]) + XCTAssertEqual(details, ["Arrows: activates Navigation", "Mods: maps f"]) + let text = AdmissionVerdictPresenter.present(verdict) { $0 == .collection(arrows.id) ? "Arrows" : "Mods" } + XCTAssertEqual(text.explanation, "Arrows and Mods both map f on the Base layer (Arrows: activates Navigation; Mods: maps f). Only one can win.") + } + // MARK: - New: environment needs func testMediaOutputIsBlockingOnTheSessionBackend() { diff --git a/Tests/KeyPathTests/RuleCollections/AdmissionToggleParityTests.swift b/Tests/KeyPathTests/RuleCollections/AdmissionToggleParityTests.swift index ec4c5f6df..71fa131d0 100644 --- a/Tests/KeyPathTests/RuleCollections/AdmissionToggleParityTests.swift +++ b/Tests/KeyPathTests/RuleCollections/AdmissionToggleParityTests.swift @@ -44,7 +44,7 @@ final class AdmissionToggleParityTests: XCTestCase { let manager = makeManager(collections: c.others, rules: c.rules) var proposed = c.candidate proposed.isEnabled = true - let saveTime = RuleCollectionDeduplicator.detectConflicts( + let saveTime = AdmissionEngine.mappingConflicts( in: c.others + [proposed] + c.rules.map { $0.asRuleCollection() } ) let toggleTime = manager.conflictInfo(for: c.candidate) diff --git a/Tests/KeyPathTests/RuleCollections/DeduplicatorEdgeCaseTests.swift b/Tests/KeyPathTests/RuleCollections/DeduplicatorEdgeCaseTests.swift index 084438834..acad26480 100644 --- a/Tests/KeyPathTests/RuleCollections/DeduplicatorEdgeCaseTests.swift +++ b/Tests/KeyPathTests/RuleCollections/DeduplicatorEdgeCaseTests.swift @@ -65,7 +65,7 @@ final class DeduplicatorEdgeCaseTests: XCTestCase { KeyMapping(input: "f", action: .keystroke(key: "g")), ]) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection1, collection2]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection1, collection2]) XCTAssertFalse(conflicts.isEmpty, "Should detect conflict on key 'a'") } @@ -77,7 +77,7 @@ final class DeduplicatorEdgeCaseTests: XCTestCase { KeyMapping(input: "s", action: .keystroke(key: "d")), ]) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection1, collection2]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection1, collection2]) XCTAssertTrue(conflicts.isEmpty, "No conflict when keys don't overlap") } diff --git a/Tests/KeyPathTests/RuleCollections/MultiCollectionConflictTests.swift b/Tests/KeyPathTests/RuleCollections/MultiCollectionConflictTests.swift index 833eae523..bc6b07a2e 100644 --- a/Tests/KeyPathTests/RuleCollections/MultiCollectionConflictTests.swift +++ b/Tests/KeyPathTests/RuleCollections/MultiCollectionConflictTests.swift @@ -42,7 +42,7 @@ final class MultiCollectionConflictTests: XCTestCase { let b = collection(name: "B", mappings: [("a", "2")]) let c = collection(name: "C", mappings: [("a", "3")]) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [a, b, c]) + let conflicts = AdmissionEngine.mappingConflicts(in: [a, b, c]) XCTAssertEqual(conflicts.count, 1) XCTAssertEqual(conflicts.first?.inputKey, "a") XCTAssertEqual(conflicts.first?.conflictingCollections.count, 3) @@ -56,7 +56,7 @@ final class MultiCollectionConflictTests: XCTestCase { let b = collection(name: "B", mappings: [("a", "3"), ("b", "4")]) let c = collection(name: "C", mappings: [("a", "5")]) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [a, b, c]) + let conflicts = AdmissionEngine.mappingConflicts(in: [a, b, c]) let conflictKeys = Set(conflicts.map(\.inputKey)) XCTAssertTrue(conflictKeys.contains("a")) XCTAssertTrue(conflictKeys.contains("b")) @@ -73,7 +73,7 @@ final class MultiCollectionConflictTests: XCTestCase { let b = collection(name: "B", mappings: [("a", "2")], enabled: false) let c = collection(name: "C", mappings: [("a", "3")]) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [a, b, c]) + let conflicts = AdmissionEngine.mappingConflicts(in: [a, b, c]) XCTAssertEqual(conflicts.count, 1) XCTAssertEqual(conflicts.first?.conflictingCollections.count, 2) XCTAssertFalse( diff --git a/Tests/KeyPathTests/RuleCollections/RuleCollectionDeduplicatorTests.swift b/Tests/KeyPathTests/RuleCollections/RuleCollectionDeduplicatorTests.swift index f1527044d..d69d3380a 100644 --- a/Tests/KeyPathTests/RuleCollections/RuleCollectionDeduplicatorTests.swift +++ b/Tests/KeyPathTests/RuleCollections/RuleCollectionDeduplicatorTests.swift @@ -23,7 +23,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .navigation ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection1, collection2]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection1, collection2]) XCTAssertEqual(conflicts.count, 1) XCTAssertEqual(conflicts.first?.inputKey, "h") @@ -47,7 +47,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .base ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection1, collection2]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection1, collection2]) XCTAssertTrue(conflicts.isEmpty) } @@ -69,7 +69,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .navigation ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection1, collection2]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection1, collection2]) XCTAssertTrue(conflicts.isEmpty) } @@ -94,7 +94,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { ) disabled.isEnabled = false - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [enabled, disabled]) + let conflicts = AdmissionEngine.mappingConflicts(in: [enabled, disabled]) XCTAssertTrue(conflicts.isEmpty) } @@ -122,7 +122,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .navigation ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection1, collection2]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection1, collection2]) XCTAssertEqual(conflicts.count, 2) let conflictKeys = conflicts.map(\.inputKey).sorted() @@ -155,7 +155,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "space", targetLayer: .custom("window")) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [navCollection, windowCollection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [navCollection, windowCollection]) // "space" normalizes to the kanata key "spc" let activatorConflict = conflicts.first { $0.inputKey == "spc" } @@ -186,7 +186,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "space", targetLayer: .navigation) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [first, second]) + let conflicts = AdmissionEngine.mappingConflicts(in: [first, second]) XCTAssertFalse( conflicts.contains { $0.inputKey == "spc" }, @@ -215,7 +215,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "tab", targetLayer: .custom("window")) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [first, second]) + let conflicts = AdmissionEngine.mappingConflicts(in: [first, second]) XCTAssertTrue(conflicts.isEmpty, "Distinct activator keys do not conflict") } @@ -245,7 +245,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "f", targetLayer: .custom("function"), sourceLayer: .navigation) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [fromBase, fromNav]) + let conflicts = AdmissionEngine.mappingConflicts(in: [fromBase, fromNav]) XCTAssertFalse( conflicts.contains { $0.inputKey == "f" }, @@ -275,7 +275,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { ) disabled.isEnabled = false - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [enabled, disabled]) + let conflicts = AdmissionEngine.mappingConflicts(in: [enabled, disabled]) XCTAssertFalse( conflicts.contains { $0.inputKey == "spc" }, @@ -302,7 +302,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "space", targetLayer: .custom("window")) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [windowCollection], leaderKey: leader ) @@ -327,7 +327,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "space", targetLayer: .navigation) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [navCollection], leaderKey: leader ) @@ -351,7 +351,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "space", targetLayer: .custom("window")) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [windowCollection], leaderKey: leader ) @@ -372,7 +372,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "space", targetLayer: .custom("window")) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [windowCollection], leaderKey: leader ) @@ -396,7 +396,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "f", targetLayer: .custom("function"), sourceLayer: .navigation) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [chained], leaderKey: leader ) @@ -419,7 +419,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .base ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [baseCollection], leaderKey: leader ) @@ -442,7 +442,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .base ) - let conflicts = RuleCollectionDeduplicator.detectConflicts( + let conflicts = AdmissionEngine.mappingConflicts( in: [baseCollection], leaderKey: leader ) @@ -475,7 +475,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "f", targetLayer: .custom("home-arrows"), sourceLayer: .base) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [homeRowMods, homeRowArrows]) + let conflicts = AdmissionEngine.mappingConflicts(in: [homeRowMods, homeRowArrows]) let conflict = conflicts.first { $0.inputKey == "f" } XCTAssertNotNil(conflict, "An activator key colliding with another collection's base mapping should surface") @@ -504,7 +504,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "f", targetLayer: .custom("arrows"), sourceLayer: .base) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [mapper, arrows]) + let conflicts = AdmissionEngine.mappingConflicts(in: [mapper, arrows]) XCTAssertTrue( conflicts.contains { $0.inputKey == "f" }, "Activator-vs-mapping conflict must be detected by identity even when collections share a name" @@ -530,7 +530,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "g", targetLayer: .custom("arrows"), sourceLayer: .base) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [mapper, arrows]) + let conflicts = AdmissionEngine.mappingConflicts(in: [mapper, arrows]) XCTAssertTrue(conflicts.isEmpty, "Activator and mapping on different keys do not conflict") } @@ -555,7 +555,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "f", targetLayer: .custom("sym"), sourceLayer: .navigation) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [mapper, chained]) + let conflicts = AdmissionEngine.mappingConflicts(in: [mapper, chained]) XCTAssertTrue(conflicts.isEmpty, "Activator from a different source layer than the mapping is valid") } @@ -572,7 +572,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { momentaryActivator: MomentaryActivator(input: "f", targetLayer: .custom("arrows"), sourceLayer: .base) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) XCTAssertFalse( conflicts.contains { $0.inputKey == "f" }, "Same-collection activator+mapping on a key is not a cross-collection conflict" @@ -598,7 +598,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { configuration: .chordGroups(config) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) let conflict = conflicts.first { $0.inputKey == "Navigation" } XCTAssertNotNil(conflict, "Duplicate chord group names should be surfaced") @@ -622,7 +622,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { configuration: .chordGroups(config) ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) XCTAssertTrue(conflicts.isEmpty, "Unique chord group names do not conflict") } @@ -645,7 +645,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .base ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) let aliasConflict = conflicts.first { $0.inputKey.contains("caps-lock") && $0.inputKey.contains("caps_lock") } XCTAssertNotNil(aliasConflict, "Two keys sanitizing to the same alias name should be surfaced") @@ -665,7 +665,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .base ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) XCTAssertTrue(conflicts.isEmpty, "Simple mappings generate no alias, so no alias collision") } @@ -683,7 +683,7 @@ final class RuleCollectionDeduplicatorTests: XCTestCase { targetLayer: .base ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collection]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collection]) XCTAssertTrue(conflicts.isEmpty, "Distinct sanitized stems don't collide") } diff --git a/Tests/KeyPathTests/Services/ConfigurationServiceSavePipelineTests.swift b/Tests/KeyPathTests/Services/ConfigurationServiceSavePipelineTests.swift index 76e757ed9..65b78ae96 100644 --- a/Tests/KeyPathTests/Services/ConfigurationServiceSavePipelineTests.swift +++ b/Tests/KeyPathTests/Services/ConfigurationServiceSavePipelineTests.swift @@ -605,7 +605,7 @@ final class ConfigurationServiceSavePipelineTests: KeyPathTestCase { isSystemDefault: false ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collA, collB]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collA, collB]) XCTAssertEqual(conflicts.count, 1) XCTAssertEqual(conflicts.first?.inputKey, "caps") } @@ -628,7 +628,7 @@ final class ConfigurationServiceSavePipelineTests: KeyPathTestCase { isSystemDefault: false ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [enabled, disabled]) + let conflicts = AdmissionEngine.mappingConflicts(in: [enabled, disabled]) XCTAssertTrue(conflicts.isEmpty, "Disabled collections should not cause conflicts") } @@ -656,7 +656,7 @@ final class ConfigurationServiceSavePipelineTests: KeyPathTestCase { isSystemDefault: false ) - let conflicts = RuleCollectionDeduplicator.detectConflicts(in: [collA, collB]) + let conflicts = AdmissionEngine.mappingConflicts(in: [collA, collB]) XCTAssertEqual(conflicts.count, 2) XCTAssertEqual(conflicts[0].inputKey, "a") XCTAssertEqual(conflicts[1].inputKey, "z") diff --git a/docs/architecture/rules-admission-review.md b/docs/architecture/rules-admission-review.md index 87ea0233e..24dd912f3 100644 --- a/docs/architecture/rules-admission-review.md +++ b/docs/architecture/rules-admission-review.md @@ -458,6 +458,35 @@ Callers, all through this one function: is what `onError`, `sessionConfigurationRefusal` and `currentRuntimeStatus` all carry, and a CLI rendering test. +### 4.4g The engine owns conflict detection (2026-10-20) + +Phase 3 had wrapped `RuleCollectionDeduplicator.detectConflicts`, which reported +*display names*, and mapped names back to rule ids — so two rules titled alike were +both blamed for one collision. Replaced, not patched: `AdmissionEngine.conflicts(in:leaderKey:)` +computes every conflict from claims — what each owner puts on which slot — and the +old detector is deleted. One pass over the enabled owners builds the stakes; the +five kinds of collision are the five ways stakes disagree: + +| slot | collision | parties | +|---|---|---| +| `(input, layer)` | two owners map it; the leader key holds its base slot (#463) | the owners, by id | +| activator `(source layer, key)` vs mappings | an activator shadows another owner's mapping (#466) | activator owner first, then mappers by name | +| activator slot | two activators name different target layers | in claim order; identical activators are redundant, not conflicting | +| chord group name / chord key, within one owner | duplicate `defchords` names (#464), a key in two groups | `.named` groups; the verdict's subject is the owning collection | +| generated alias name | distinct inputs sanitise to one alias (#462) | owners by name | + +`Reason.conflict` gained `details` (hold action, "activates X", alias origin), so the +presenter and the save gate's `MappingConflictInfo` say the same thing. +`mappingConflicts(in:leaderKey:)` is the bridge to that error type for the save gate +(`ConfigurationService.generateConfiguration`) and the CLI (`RulesFacade`); both are +`nonisolated`, so the gate needs no main-actor hop. The deduplicator keeps only +`dedupe` — the generator's structural "first claim wins" — which now runs after +admission has refused every intentional overlap and warns if it ever drops anything. +The detector's test corpus (`RuleCollectionDeduplicatorTests`, `MultiCollectionConflictTests`, +`AliasDeduplicationTests`, the CLI and save-pipeline tests) now runs against the engine +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.5 Lowering: intent in rules, backend output in the generator Rules keep expressing intent (`volu`, `caps` → Hyper, "this keyboard only").