feat(#760): Wire channel scope into rule matching (#751)#777
feat(#760): Wire channel scope into rule matching (#751)#777amiable-dev merged 6 commits intomainfrom
Conversation
Channel scope on DeviceIdentityConfig now filters events at runtime. Events with channels outside the binding's scope skip device-specific rules while any-device rules still apply. - Add ProcessedEvent::channel() extractor for all variants - Add channel_scopes HashMap on CompiledRuleSet (populated by compiler) - is_channel_in_scope() check in match_event() gates device-specific rules - Rule compiler builds scopes from Config.devices[].channels - TDD: 2 new tests (channel filtering, empty scope = all channels) 8 routing tests pass (6 existing + 2 new). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR wires the existing per-device channels scope (DeviceIdentityConfig.channels) into the compiled rule matching pipeline so device-scoped mappings only apply when an event’s MIDI channel is within that device’s configured scope.
Changes:
- Add
ProcessedEvent::channel()to consistently extract the MIDI channel across processed event variants. - Compile per-device channel scopes into
CompiledRuleSetand gate device-specific rule matching based on scope. - Add routing tests covering scoped vs unscoped device channel behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| conductor-core/src/event_processor.rs | Adds ProcessedEvent::channel() helper used by channel scope filtering. |
| conductor-core/src/rule_compiler.rs | Builds per-device channel_scopes from Config.devices[].channels and attaches them to the compiled ruleset. |
| conductor-core/src/rule_set.rs | Applies channel-scope gating to device-specific branches in match_event(). |
| conductor-core/tests/multi_device_rule_routing_test.rs | Adds tests validating scoped device matching behavior (accept/reject by channel). |
conductor-core/src/rule_set.rs
Outdated
| /// Set channel scopes from device identity configs (#751) | ||
| pub(crate) fn set_channel_scopes(&mut self, scopes: HashMap<String, Vec<u8>>) { | ||
| self.channel_scopes = scopes; | ||
| } |
There was a problem hiding this comment.
CompiledRuleSet is documented as “immutable” / “never mutated after construction”, but the new set_channel_scopes(&mut self, ...) mutator violates that invariant and makes it easier to accidentally mutate a rule set after it has been swapped into ArcSwap. Prefer passing channel_scopes into CompiledRuleSet::new(...) (or building it inside new) and remove the setter to keep the type construction-only and truly immutable.
| input: None, | ||
| output: None, | ||
| protocol: None, | ||
| channels: vec![9], // Only channel 10 (0-indexed) |
There was a problem hiding this comment.
The inline comment is internally inconsistent: channels: vec![9] is “channel 9 (0-indexed)” which corresponds to MIDI channel 10 in 1-indexed UI terms, but the comment currently says “Only channel 10 (0-indexed)”. Please correct the wording to avoid confusion about the 0- vs 1-indexed convention.
| channels: vec![9], // Only channel 10 (0-indexed) | |
| channels: vec![9], // Only channel 9 (0-indexed, i.e., MIDI channel 10) |
conductor-core/src/rule_set.rs
Outdated
| // Channel scope check (#751): if device has a channel scope and this | ||
| // event's channel is outside it, skip all device-specific matching. | ||
| // Any-device rules and global rules still apply (they're not device-scoped). | ||
| let device_in_scope = | ||
| device_id.is_none_or(|device| self.is_channel_in_scope(device, event)); | ||
|
|
||
| // Check mode rules if mode_index is valid | ||
| if let Some(mode) = self.mode_rules.get(mode_index) { | ||
| // 1. Device-specific rules for this mode | ||
| if let Some(device) = device_id | ||
| // 1. Device-specific rules for this mode (skip if channel out of scope) | ||
| if device_in_scope | ||
| && let Some(device) = device_id | ||
| && let Some(rules) = mode.device_rules.get(device) | ||
| && let Some(action) = find_matching_action(rules, event) | ||
| { |
There was a problem hiding this comment.
Channel scoping is only applied in match_event(). match_event_with_provenance() still evaluates device-specific rules even when the event channel is out of scope, which can cause actions to fire when the daemon uses provenance matching (e.g., in event monitoring/capture paths). Apply the same device_in_scope gating to the device-specific branches in match_event_with_provenance() (and add/adjust a test to cover this case).
…e, comment - Pass channel_scopes into CompiledRuleSet::new() instead of setter - Apply channel scope check to match_event_with_provenance() too - Fix comment: channel 9 = 0-indexed (MIDI channel 10) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
conductor-core/src/rule_compiler.rs
Outdated
| // Build channel scopes from device identity configs (#751) | ||
| let channel_scopes: HashMap<String, Vec<u8>> = config | ||
| .devices | ||
| .iter() | ||
| .filter(|d| !d.channels.is_empty()) | ||
| .map(|d| (d.alias.clone(), d.channels.clone())) | ||
| .collect(); |
There was a problem hiding this comment.
compile() filters out devices with empty channels, so channel_scopes will never contain an entry with an empty vec. This makes the "Empty vec = all channels" semantics depend on absence of a key rather than an empty value; consider either (a) collecting all devices (including empty channels) so the data matches the documented semantics, or (b) updating the documentation/comments to reflect that missing key means "all channels".
| /// Check if an event's channel is in scope for a device (#751) | ||
| fn is_channel_in_scope(&self, device_id: &str, event: &ProcessedEvent) -> bool { | ||
| match self.channel_scopes.get(device_id) { | ||
| Some(channels) if !channels.is_empty() => { | ||
| match event.channel() { | ||
| Some(ch) => channels.contains(&ch), | ||
| None => true, // Non-MIDI events bypass channel filtering | ||
| } | ||
| } | ||
| _ => true, // No scope or empty scope = all channels | ||
| } | ||
| } |
There was a problem hiding this comment.
CompiledRuleSet::is_channel_in_scope() duplicates the channel-scope semantics already implemented on DeviceIdentityConfig::is_channel_in_scope(). To reduce the risk of these two implementations drifting over time, consider extracting a shared helper (e.g., a small function that takes &[u8] + Option<u8>), and have both places call it.
… doc - Extract channel_in_scope() as shared helper in rule_set.rs - DeviceIdentityConfig::is_channel_in_scope() delegates to shared helper - Document that missing key in channel_scopes = no restriction - Prevents logic drift between two implementations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
conductor-core/src/rule_set.rs
Outdated
| global_rules: GlobalRuleSet, | ||
| /// Monotonic version for debugging/logging | ||
| version: u64, | ||
| /// Channel scopes per device alias (#751). Empty vec = all channels. |
There was a problem hiding this comment.
The struct field doc says "Empty vec = all channels", but rule_compiler::compile() intentionally does not store entries for devices with empty channels (it omits the key entirely). Consider updating this comment to reflect the actual invariant (e.g., missing key means no restriction, and an empty vec would also mean no restriction if ever stored).
| /// Channel scopes per device alias (#751). Empty vec = all channels. | |
| /// Channel scopes per device alias (#751). | |
| /// Missing key = no restriction (all channels); an empty vec would also mean no restriction if present. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
conductor-core/src/rule_set.rs
Outdated
| // Channel scope check (#751): if device has a channel scope and this | ||
| // event's channel is outside it, skip all device-specific matching. | ||
| // Any-device rules and global rules still apply (they're not device-scoped). | ||
| let device_in_scope = | ||
| device_id.is_none_or(|device| self.is_channel_in_scope(device, event)); | ||
|
|
There was a problem hiding this comment.
Channel scoping is intended to skip only device-specific rule lookups while still allowing mode/global any-device rules to match. The new tests cover device-specific rejection/acceptance, but there’s no assertion that an out-of-scope event still matches an any-device rule (i.e., a trigger with no device filter). Adding a test for that interaction would prevent regressions to the stated behavior.
Verifies that when a device's channel scope rejects an event, the any-device rules in the same mode still fire. This covers the interaction between channel scoping and rule priority ordering. 9 routing tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
conductor-core/src/rule_set.rs
Outdated
| // Channel scope check (#751) — same as match_event() | ||
| let device_in_scope = | ||
| device_id.is_none_or(|device| self.is_channel_in_scope(device, event)); | ||
|
|
There was a problem hiding this comment.
The channel-scope gating logic is duplicated in both match_event() and match_event_with_provenance(). To reduce the risk of future drift (e.g., behavior changes made in one but not the other), consider factoring this into a small shared helper (or reusing match_event() and then enriching provenance).
…venance (round 5) Both methods apply the same channel scope gating pattern. Comments cross-reference each other to prevent drift during future changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Wire the existing
channelsfield onDeviceIdentityConfiginto the runtime rule matching pipeline. Events with channels outside a binding's scope now skip that binding's device-specific rules.Changes
ProcessedEvent::channel()method — extracts MIDI channel from all variantschannel_scopes: HashMap<String, Vec<u8>>toCompiledRuleSetConfig.devices[].channelsmatch_event()gates device-specific rule lookup withis_channel_in_scope()TDD
test_channel_scope_filters_events: device withchannels: [9]rejects ch.0, accepts ch.9test_empty_channel_scope_matches_all: empty channels = all channels matchTest plan
cargo clippy --workspacecleancargo test --workspaceNote
This PR is independent of #776 (UsbIdentifier wiring) — no merge conflicts. Both can be reviewed/merged in parallel.
🤖 Generated with Claude Code