feat: add definputdevices and (device N) switch conditions

[malpern]

Describe the changes

Add definputdevices and (device N) switch conditions for per-device mappings.

definputdevices maps user-chosen numeric device IDs to matcher lists. (device N) checks whether the current input event came from device N.

Implement the end-to-end path on macOS by matching definputdevices entries against connected devices at startup, then tagging each input event with the configured device ID before switch evaluation.

Also bump karabiner-driverkit to 0.3.0 so macOS input events expose device_hash, which is used to translate matched devices into per-event IDs.

Related to jtroo#1944 and the earlier attempt in jtroo#1974.

Checklist

• Add documentation to docs/config.adoc
  • Done
• Add example and basic docs to cfg_samples/kanata.kbd
  • Done
• Update error messages
  • Added parse errors for invalid IDs, missing definputdevices, unknown IDs, and duplicate matcher properties
• Added tests, or did manual testing
  • Added switch_device_match and switch_device_opcode_roundtrip
  • Workspace tests pass
  • Manual macOS testing completed with multiple keyboards

[claude]

Code Review: definputdevices config block and (device N) switch condition

Overall this is a clean, well-structured PR that follows the existing codebase conventions. The architecture is sound — using NonZeroU8 for IDs is a nice touch, and the two-opcode sentinel pattern for DEVICE_VAL is consistent with LAYER_VAL / BASE_LAYER_VAL. A few things worth addressing before merging:

---

Bugs / Correctness

1. vendor_id / product_id should be u16, not u64

parser/src/cfg/definputdevices.rs, lines 9-10:

pub vendor_id: Option<u64>,
pub product_id: Option<u64>,

USB VID and PID are 16-bit values (0x0000–0xFFFF). Typing them as u64 lets the parser silently accept clearly invalid inputs like (vendor_id 0xDEADBEEFCAFE) today, and will produce a confusing mismatch when the actual matching logic is wired up. Change these to u16 and update parse_hex_or_decimal accordingly — the error messages already show 4-nibble examples (0x1D50, 0x615E).

---

Code Quality

2. Dead has_any check

definputdevices.rs, lines 97–106:

let has_any = matcher.name.is_some() || ...;
if !has_any {
    bail_expr!(..., "device matcher must specify at least one property");
}

This can never trigger. The guard at line 41 already bails on an empty matcher_list, and every iteration of the loop either sets a field or bails via bail_expr! on an unknown property. After the loop, has_any is always true. Remove these lines to avoid misleading readers.

3. Unnecessary .clone() when populating ParserState

parser/src/cfg/mod.rs:

input_devices: input_devices.clone(),

The input_devices local is not used again after being moved into ParserState, so this clone allocates a second HashMap for no reason. Drop the .clone() and let the move happen.

4. Inline std::num::NonZeroU8 is verbose

keyberon/src/action/switch.rs and keyberon/src/layout.rs use the full path std::num::NonZeroU8 several times each. Adding use std::num::NonZeroU8; at the top of those files (matching the existing use in definputdevices.rs) would clean this up.

---

Missing Tests

5. No tests for the new functionality

This is the most significant gap. The PR adds a new opcode, a new parse path, and a new boolean condition — but zero new tests. For a hot-path keyboard engine, this is risky. Suggested test cases:

• evaluate_boolean with Device opcode: verify device_id == Some(id) returns true, device_id == Some(other_id) returns false, and device_id == None returns false.
• parse_definputdevices: valid input, duplicate ID error, ID=0 error, missing matcher list, unknown property, empty matcher list.
• (device N) in switch without definputdevices: should produce a parse error.
• (device N) where N is not defined in definputdevices: should produce a parse error.
• End-to-end sim test: config with definputdevices + a switch using (device N), exercised by the sim test harness with a KeyEvent carrying a device_id.

---

Design / Future-proofing

6. The feature is a compile-time and parse-time-only stub at runtime

definputdevices is fully parsed and (device N) opcodes are emitted and evaluated, but event.device_id is None on every platform right now. The switch condition can therefore never match. That's fine for groundwork, but it would help to add a // TODO comment at the device_id: None assignment sites (or in handle_input_event) so it's obvious to readers why the feature appears wired up but doesn't fire.

7. current_device_id is an unrestricted public field

keyberon/src/layout.rs:

pub current_device_id: Option<std::num::NonZeroU8>,

Setting this is the entire mechanism for routing per-device behaviour. Exposing it as a bare pub field is consistent with some other fields in Layout, so this is low priority — but it's worth noting for the upstream review since it bypasses any future invariants you might want to enforce.

---

Minor

• The DEVICE_VAL = 855 sentinel sits correctly in (KEY_MAX=850, MAX_OPCODE_LEN=0x0FFF] — this range check is fine.
• The expect("device ID must be nonzero") during opcode decoding is safe because new_device enforces it at construction, and the as u8 truncation of op2.0 is sound since IDs ≤ 255 fit in both u8 and u16.
• The duplicate-definputdevices-block check using .nth(1) on the spanned iterator is consistent with how other top-level blocks are guarded.

---

Good foundation — the parse-time validation (especially the "must be defined in definputdevices" check on (device N)) is exactly the right approach. Once the missing tests are added and the u64 type is tightened to u16, this will be in good shape for the upstream PR.

[claude]

Code Review

Nice work on the architecture — the NonZeroU8 ID type, parse-time validation, and the two-opcode sentinel pattern (consistent with LAYER_VAL/BASE_LAYER_VAL) are all solid design choices. A few issues worth addressing before upstreaming:

---

Bugs / Correctness

1. Hash matching uses substring comparison (macos.rs:841)

let hash_matches = matcher
    .hash
    .as_ref()
    .map_or(true, |h| format!("{:x}", kb.hash).contains(h.as_str()));

This means (hash cd81) would match a device with hash acd81ef0. The config example shows (hash cd81ef) suggesting a full/exact hex match is intended. I'd recommend == after formatting both to the same length, or at minimum document that partial prefix matching is intentional.

2. Vendor/product ID range not validated (definputdevices.rs:341-354)

vendor_id and product_id are stored as u64 but USB IDs are u16 (max 0xFFFF). parse_hex_or_decimal accepts 0x1_0000 without error. This means a typo like 0x11D50 silently creates a matcher that can never match. Either:

• Store as u16 (or u32 for HID usage page support) and validate at parse time, or
• At minimum add a bounds check and emit an error

3. Duplicate property silently overwrites (definputdevices.rs:333-362)

1 ((name "Kinesis") (name "Advantage360"))

The second name silently replaces the first. Should bail_expr! if a field is set twice.

4. Multi-ID match for a single device takes last writer silently (macos.rs:854)

In build_device_hash_to_id_map, if one physical device satisfies matchers for both ID 1 and ID 2, the loop will insert both, and the last one wins with no warning. A log::warn! here would help users diagnose misconfigured matchers.

---

Design / API

5. has_any check is unreachable dead code (definputdevices.rs:364-373)

The matcher_list.is_empty() guard above already ensures at least one matcher item exists. Each item must have a recognized property (unknown ones bail), and all recognized properties set a field, so has_any is always true here. Remove it.

6. input_devices_vec intermediate conversion is unnecessary (macos.rs:601-604)

let input_devices_vec: Option<Vec<_>> = k
    .input_devices
    .as_ref()
    .map(|m| m.iter().map(|(k, v)| (*k, v.clone())).collect());

This clones all the matchers just to pass them as a slice. You could pass Option<&HashMap<...>> directly to KbdIn::new and iterate in build_device_hash_to_id_map, which would avoid the allocation and the clone.

7. Hot-plugging not supported — should document

The device_hash → id map is built once at startup. A keyboard plugged in after kanata starts will not be recognized. This is a reasonable limitation for an initial implementation, but it should be documented (e.g., a log::warn! if a lookup miss occurs, or a note in the config docs).

---

Testing

8. No unit tests for (device N) switch evaluation

All the existing evaluate_bool_test / evaluate_boolean call sites just pass None for device_id. There are no tests exercising the actual Device(id) opcode path. Consider adding:

#[test]
fn switch_device_id() {
    let id1 = NonZeroU8::new(1).unwrap();
    let id2 = NonZeroU8::new(2).unwrap();
    let (op1, op2) = OpCode::new_device(id1);
    assert!(evaluate_boolean(&[op1, op2], ..., Some(id1)));
    assert!(!evaluate_boolean(&[op1, op2], ..., Some(id2)));
    assert!(!evaluate_boolean(&[op1, op2], ..., None));
}

9. No integration test for definputdevices parsing

A test in sim_tests or similar that exercises parse-time errors (unknown ID in (device N), missing definputdevices, duplicate ID) would prevent regressions and serve as documentation.

---

Minor / Style

10. std::num::NonZeroU8 repeated inline throughout — consider use std::num::NonZeroU8; at the top of each file where it's used more than once (switch.rs, macos.rs, definputdevices.rs, etc.)

11. current_device_id placement — setting it as mutable state on Layout via self.layout.bm().current_device_id = event.device_id works correctly for synchronous processing but is easy to misuse. A doc comment explaining the invariant ("must be set immediately before processing each event") would help future maintainers.

---

Overall this is a clean implementation of a complex feature. The main things I'd want addressed before merge: the hash substring matching bug (#1), vendor/product ID range validation (#2), and adding at least basic unit tests for the new opcode path (#8).

[claude]

Code Review: definputdevices + (device N) switch condition

Great work building on the upstream review feedback — the use of NonZeroU8, parse-time validation, and the sentinel opcode approach are all solid. Here are my findings:

---

Bugs / Correctness

1. Redundant (and potentially incorrect) cast in macOS device matching

In src/oskbd/macos.rs, build_device_hash_to_id_map:

let vendor_matches = matcher.vendor_id.map_or(true, |v| u16::from(kb.vendor_id as u16) == v);
let product_matches = matcher.product_id.map_or(true, |p| u16::from(kb.product_id as u16) == p);

u16::from(x: u16) is a no-op identity. If kb.vendor_id / kb.product_id are wider types (e.g. u32), the as u16 truncation is silent. Should be: kb.vendor_id as u16 == v (or better, check the actual type and use try_into().unwrap_or(0)). Worth double-checking the DKEvent field types in karabiner-driverkit 0.3.0.

2. Config reload doesn't update the macOS device_hash_to_id map

build_device_hash_to_id_map is called once in KbdIn::new(). But in kanata/mod.rs:

self.input_devices = cfg.input_devices;  // updated on reload

...the KbdIn instance (which owns the hash→ID map) is not re-created on config reload. After a hot reload, device matching will use the stale map. Either re-build the map on reload (requires access to KbdIn from the reload path) or document this limitation.

3. hash stored as String — invalid hex silently never matches

InputDeviceMatcher.hash is Option<String>. At match time in build_device_hash_to_id_map:

let device_hash = format!("{:x}", kb.hash);
device_hash == *h

If the user writes (hash cd81ef) but the actual hash formats with uppercase or zero-padding, it silently fails. Worse, if the stored string contains 0x prefix (which the parser strips via strip_prefix… but wait, it doesn't strip for hash — parse_hex_or_decimal_u16 is only used for vendor/product IDs). The hash matcher has no hex parsing at all — it's a raw string compare. This should either:

• Parse the hash at config-load time (into u64) and compare numerically, or
• At minimum, document the exact expected format and normalize to lowercase.

4. Non-deterministic multi-match "winner" in macOS

In build_device_hash_to_id_map, when a device matches multiple IDs:

map.insert(kb.hash, *id);  // overwrites previous

The iteration order over the caller-supplied slice depends on how the HashMap was drained when building the Vec in macos.rs. HashMap iteration is non-deterministic, so the "last writer wins" behaviour is undefined. The warning is good, but users may find the result unpredictable. Consider using the lowest ID (most explicit wins) or making it a hard error.

---

Design / API

5. current_device_id as a mutable public field on Layout

Setting self.layout.bm().current_device_id = event.device_id before each event is an easy-to-forget contract. A handle_input_event(event, device_id) wrapper or setting it inside keyberon's process_event would be safer. At minimum the field should have a doc comment warning "must be set before each call to process".

6. panic! / .expect() in the hot-path opcode decoder

In keyberon/src/action/switch.rs:

DEVICE_VAL => OpCodeType::Device(
    NonZeroU8::new(op2.0 as u8).expect("device ID must be nonzero"),
),

This is called on every keypress that triggers a switch. The invariant is guaranteed by the parser, but if opcodes are ever deserialized/loaded from disk (future work), this becomes a DoS vector. Consider returning an error or an OpCodeType::Invalid sentinel instead, consistent with how other hot-path decode errors are handled elsewhere.

7. Linux: silent non-operation with no user warning

The PR notes Linux matching is follow-up work, but currently:

• definputdevices parses successfully on Linux
• (device N) conditions are valid syntax and always evaluate to false (since device_id is always None)

A user on Linux could spend hours debugging why their (device 1) switch branch never fires. Consider emitting a log::warn! once in handle_input_event the first time a non-None device ID is expected but unavailable, or a warning at parse time on non-macOS platforms.

---

Minor / Nits

8. Duplicated ID-parsing logic between definputdevices.rs and switch.rs

Both files independently parse a u8 from a string and validate NonZeroU8. This could be a shared helper (e.g., parse_device_id(expr) -> Result<NonZeroU8>).

9. parse_hex_or_decimal_u16 returns Box<dyn Error>

Everything else in the parser uses anyhow. The Box<dyn Error> return is inconsistent and forces the callers to use .map_err(|_| ...). Return anyhow::Result<u16> to stay consistent.

10. From<InputEvent> for DKEvent sets device_hash: 0

This is correct for output events (they don't need a device hash), but the silent zeroing looks surprising. A comment like // device_hash is irrelevant for output events would help future readers.

---

What's Good

• NonZeroU8 for IDs is an excellent choice — Option<NonZeroU8> fitting in 1 byte is a real win for the layout's hot path.
• Parse-time validation that (device N) references a defined ID is exactly right.
• DEVICE_VAL = 855 following the existing sentinel pattern keeps the opcode format consistent.
• The two-opcode instruction layout (consistent with LAYER_VAL / BASE_LAYER_VAL) is the right approach.
• Good test coverage for the new switch opcodes (match, non-match, None, roundtrip).
• The warning log on ambiguous device matching is a nice UX touch.

---

Summary: Items 1–4 are worth fixing before merge; items 5–7 are real concerns but could be follow-up issues. The rest are minor polish.

[claude]

Code Review — feat: add definputdevices config block and (device N) switch condition

Overall this is a well-structured implementation that follows existing kanata patterns. The use of NonZeroU8 for IDs, the sentinel opcode approach (mirroring LAYER_VAL/BASE_LAYER_VAL), and the parse-time validation are all solid choices. Below are observations organized by category.

---

Potential Bugs

1. Silent case-sensitive hash comparison (src/oskbd/macos.rs)

let hash_matches = matcher.hash.as_ref().map_or(true, |h| {
    let device_hash = format!("{:x}", kb.hash);
    device_hash == *h
});

format!("{:x}", ...) produces lowercase hex. If a user writes (hash CD81EF) (uppercase), it silently fails to match. The stored hash string is never normalized at parse time. Consider either lowercasing the matcher value at parse time (prop_val.to_lowercase()) or using a case-insensitive comparison. The docs show (hash cd81ef) but don't warn about case sensitivity.

2. Suspicious no-op cast (src/oskbd/macos.rs)

let vendor_matches = matcher.vendor_id.map_or(true, |v| u16::from(kb.vendor_id as u16) == v);
let product_matches = matcher.product_id.map_or(true, |p| u16::from(kb.product_id as u16) == p);

u16::from(x as u16) — if kb.vendor_id/kb.product_id are wider than u16 (e.g., u32), the as u16 silently truncates and the outer u16::from() is a no-op. If they're already u16, the u16::from() is redundant. Worth checking the actual types from karabiner-driverkit 0.3.0 and removing one of the conversions.

3. Last-match-wins overwrite when two matchers match the same device

The outer loop iterates over matchers, inner over devices. If matcher ID 1 and matcher ID 2 both match device X, the one appearing last in the config wins (no stable priority). The warning log says "using {id}" (last seen), but the iteration order may surprise users. Consider documenting this behavior explicitly or erroring on ambiguous configs.

---

Code Quality / Rust Practices

4. pub current_device_id on Layout — fragile temporal coupling

// src/kanata/mod.rs
self.layout.bm().current_device_id = event.device_id;

Setting a public field immediately before calling an event handler is a temporal coupling that compiles fine but is easy to miss in future callers. Any code that calls into layout directly (tests, external keyberon users) without setting this field first will silently get None (which is safe, but potentially wrong). Worth a doc comment on the field itself explaining the contract, or a // IMPORTANT: must be set before ... comment at the call site. The doc comment already on the field is good — just ensure callers know they own this responsibility.

5. Duplicate device ID parsing logic

The "parse atom → u8 → NonZeroU8" pattern appears identically in both definputdevices.rs and switch.rs. Extracting a parse_device_id(expr) -> Result<NonZeroU8> helper would reduce duplication and keep error messages in sync.

6. #[allow(clippy::too_many_arguments)]

The evaluate_boolean and Switch::actions functions now take 8 parameters. This PR adds one more to an already-large signature. If this PR feeds into upstream jtroo/kanata, they've historically been careful about function signatures. A SwitchEvalContext<'_> struct grouping the less-varied arguments could help, though I recognize this is a pre-existing issue that this PR shouldn't necessarily fix alone.

7. device_hash: 0 in output conversion paths

// From<InputEvent> for DKEvent
device_hash: 0,
// InputEvent::try_from(KeyEvent {...}) in write_key()
device_hash: 0,

These are correct (output events don't need a source device), but a comment like // not applicable for output events on at least one of them would prevent future confusion.

---

Performance (Hot Path)

The changes here look thoughtful:

• Option<NonZeroU8> is 1 byte (niche optimization) — negligible addition to KeyEvent.
• device_id_for_hash() is a single HashMap<u64, NonZeroU8> lookup per event — effectively free.
• evaluate_boolean receives device_id by value (1 byte copy) — no concern.
• build_device_hash_to_id_map allocates strings (format!("{:x}", ...)) but only at startup — fine.

No hot-path performance concerns introduced.

---

Test Coverage

The two new unit tests (switch_device_match, switch_device_opcode_roundtrip) cover the core opcode layer well. Gaps:

• No parser-level test for definputdevices — duplicate ID, empty matcher list, unknown property, missing matcher list, 0 as ID. These are all parse-time errors that deserve explicit test coverage in the parser tests.
• No sim test exercising the full path: config with definputdevices + (device N) in switch + events with device_id set. A sim test with device_id set via the simulated input format would give high confidence the wiring in handle_input_event → layout.current_device_id → evaluate_boolean is correct end-to-end.
• Hash case sensitivity — a test that (hash CD81EF) vs (hash cd81ef) behave consistently (whatever you decide they should do).

---

Minor / Nits

• In parse_definputdevices, the loop { let Some(...) = exprs.next() else { break }; ... } pattern is valid but a while let loop over pairs would be more idiomatic and visually cleaner.
• The sample config comment says Currently macOS only. — worth adding a similar note to the error message when (device N) fails at runtime on Linux (a warning-level log when device_id is None but a device matcher is defined could aid debugging).
• input_devices field type on Kanata and Cfg uses std::num::NonZeroU8 fully qualified while the switch code imports it. Consistency not critical, just a readability note.

---

Summary

The architecture is clean and the macOS end-to-end path looks correct. The three actionable bugs (hash case sensitivity, suspicious cast, last-match overwrite semantics) are worth fixing before merge. The test gaps — particularly a sim test and parser-level tests — would significantly increase confidence.

[claude]

Code Review: definputdevices and (device N) switch condition

This is a well-structured feature with a clean design. The use of NonZeroU8 for device IDs is smart (fits in Option<NonZeroU8> = 1 byte), the sentinel opcode approach is consistent with the existing LAYER_VAL/BASE_LAYER_VAL pattern, and parse-time validation (unknown IDs, missing block, duplicate properties) is solid. Here are my observations, roughly in priority order:

---

Bugs / Correctness

1. Live-reload doesn't update KbdIn.device_hash_to_id (silent mismatch)

In src/kanata/mod.rs, the reload path does:

self.input_devices = cfg.input_devices;

But KbdIn already has a baked-in device_hash_to_id map built at startup and never refreshed. If a user adds/changes a definputdevices entry and live-reloads, the logical config (self.input_devices) changes but the runtime hash→id lookup stays stale. The docs say "Live reload does not re-read device mappings" — but this means the (device N) conditions silently stop matching or match the wrong IDs after reload. This should either be documented more prominently, or the reload should rebuild the map (or at least warn when the two diverge).

2. Hash string comparison is case-sensitive

In src/oskbd/macos.rs:

let device_hash = format!("{:x}", kb.hash);
device_hash == *h

{:x} produces lowercase hex. If a user writes (hash "A1B2C3") (from copy-pasting a macOS system profiler output) instead of (hash "a1b2c3"), the matcher silently fails. Consider normalizing both sides to lowercase:

device_hash.eq_ignore_ascii_case(h.as_str())

3. vendor_id as u16 / product_id as u16 silent truncation

let vendor_matches = matcher.vendor_id.is_none_or(|v| kb.vendor_id as u16 == v);

Depending on the type of kb.vendor_id from the driverkit crate, casting as u16 can silently truncate. Check that this type is already u16 or u32 ≤ 65535 range-guaranteed; if not, use a saturating cast or an explicit range check to avoid surprising non-matches.

---

Design Concerns

4. (device N) silently never matches on Linux/Windows

On non-macOS platforms, device_id in KeyEvent is always None, so (device N) in a switch condition always evaluates to false — silently. A user who writes a config expecting this to work cross-platform gets no error. Consider emitting a parse-time warning (or error) when (device N) is used without macOS being the target platform, at least via a note in the error message referencing the OS limitation.

5. current_device_id is a pub mutable field on Layout

pub current_device_id: Option<std::num::NonZeroU8>,

This is set via self.layout.bm().current_device_id = event.device_id; immediately before event processing. Making it a pub field means it can be accidentally modified from anywhere. A set_current_device_id(id) method would make the intent clearer and protect the invariant that it's always set immediately before each event.

6. #[allow(clippy::too_many_arguments)] on hot-path functions

Both evaluate_boolean and actions() suppress too_many_arguments. These functions are on the critical path for every keypress. A SwitchContext struct grouping layers, default_layer, device_id, and the historical iterators would clean this up and make future extensions (e.g., more switch conditions) non-breaking. This is a refactor suggestion, not a blocker for this PR.

---

Minor Issues

7. Hash comparison allocates per device per startup check

let device_hash = format!("{:x}", kb.hash);

This allocates a String for every (id, device) pair at startup. Not a hot-path concern (it's one-time), but write! into a pre-allocated buffer or format_args! would avoid it.

8. From<InputEvent> for DKEvent hardcodes device_hash: 0

This is on the output/write path so 0 is a harmless sentinel, but it's worth a comment explaining why (we're generating synthetic output events that don't originate from a physical device).

9. parse_hex_or_decimal_u16 uses Box<dyn Error>

The return type std::result::Result<u16, Box<dyn std::error::Error>> is unusual for an internal parser helper. A simple Option<u16> or a dedicated ParseIntError type would be cleaner.

---

Test Coverage

The two new tests (switch_device_match, switch_device_opcode_roundtrip) are good. Suggested additions:

• A test for (device N) with not operator wrapping it (since Not is applied after the opcode is evaluated).
• A test for the parser error cases: using (device N) without a definputdevices block, and referencing an undefined ID.
• A test for the hash case-sensitivity issue mentioned above (once fixed).

---

Summary

Category	Item	Severity
Bug	Live-reload leaves KbdIn.device_hash_to_id stale	Medium
Bug	Hash comparison is case-sensitive	Medium
Bug	as u16 silent truncation risk	Low–Medium
Design	(device N) silently fails on non-macOS	Medium
Design	current_device_id as public mutable field	Low
Design	too_many_arguments suppressions on hot path	Low
Minor	Per-startup string allocation for hash comparison	Negligible
Minor	Missing comment on device_hash: 0 sentinel	Negligible

The live-reload staleness and hash case-sensitivity are the two issues most likely to create confusing user-facing behavior. The rest are either low-risk or polish items. Overall the architecture is sound and the implementation is clean.

[claude]

Code Review: definputdevices + (device N) switch condition

Overall this is a clean, well-scoped design. The NonZeroU8 ID type, parse-time validation, AND-semantics for matchers, and consistent opcode sentinel approach are all solid choices. A few issues worth addressing before merging:

---

🐛 Bug: Hash comparison is case-sensitive

In src/oskbd/macos.rs, device hashes are formatted as lowercase hex:

let device_hash = format!("{:x}", kb.hash);
device_hash == *h

But a user writing (hash "ABCDEF1234") in their config (uppercase) will silently never match. Either normalize both sides to lowercase, or document the requirement and reject uppercase in the parser with a clear error.

---

🐛 Potentially truncating cast in vendor/product matching

let vendor_matches = matcher.vendor_id.is_none_or(|v| kb.vendor_id as u16 == v);
let product_matches = matcher.product_id.is_none_or(|p| kb.product_id as u16 == p);

If kb.vendor_id / kb.product_id are wider than u16 (e.g. u32 from the driverkit FFI), the as u16 cast will silently truncate and can produce false positives. Use u16::try_from(kb.vendor_id).map_or(false, |v| v == matcher_v) to make it explicit.

---

⚠️ Design concern: current_device_id as mutable public field

In src/kanata/mod.rs:

self.layout.bm().current_device_id = event.device_id;

Setting state on the layout externally before each event is fragile:

• The field is pub, so anything can accidentally clobber or read it at the wrong time.
• If handle_input_event panics or returns early, the stale value persists until the next event.

A cleaner approach would be to pass device_id explicitly into the event-processing path, or at minimum make current_device_id non-pub and expose a setter. Compare with how default_layer is passed through to Switch::actions as a regular parameter rather than via ambient state.

---

⚠️ Live reload silently diverges

src/kanata/mod.rs updates self.input_devices on live reload:

self.input_devices = cfg.input_devices;

But KbdIn's device_hash_to_id map (built at startup from fetch_devices()) is not rebuilt. So after a live reload with changed definputdevices, self.input_devices reflects the new config but lookups still use the old map. The NOTE in the docs says this, but the behavior is surprising — it could be worth either re-building the map on reload or logging a warning when input_devices changes on reload.

---

⚠️ (device N) silently always-false on non-macOS

The definputdevices block and (device N) syntax are parsed and accepted on all platforms, but device_id is always None on Linux and Windows, so the condition always evaluates to false without any warning. A user testing their config on macOS and deploying to Linux will see silent breakage. Consider:

• A parse-time log::warn! when definputdevices is used on a non-macOS platform, or
• A compile-time #[cfg] error/warning path.

---

🔧 Minor: redundant Vec allocation in macos.rs

let input_devices_vec: Option<Vec<_>> = k
    .input_devices
    .as_ref()
    .map(|m| m.iter().map(|(k, v)| (*k, v.clone())).collect());
drop(k);
let mut kb = match KbdIn::new(include_names, exclude_names, input_devices_vec.as_deref()) {

The HashMap is cloned into a Vec just to pass a slice reference to KbdIn::new. Changing KbdIn::new's signature to accept Option<&HashMap<NonZeroU8, InputDeviceMatcher>> directly (and iterating it inside build_device_hash_to_id_map) avoids the intermediate allocation.

---

🔧 Minor: #[allow(clippy::too_many_arguments)] accumulation

Both Switch::actions and evaluate_boolean now carry this suppressor. The hot-path evaluation function has 8 parameters. Grouping the evaluation context (layers, default_layer, device_id, etc.) into a SwitchEvalCtx struct would clean this up and make future additions easier — though it's not blocking.

---

✅ Good things to call out

• NonZeroU8 fitting Option<NonZeroU8> in one byte is a nice use of the type system.
• Parse-time validation that (device N) IDs are declared in definputdevices prevents silent runtime failures.
• The duplicate-property and duplicate-ID error messages are clear and helpful.
• switch_device_match and switch_device_opcode_roundtrip tests cover the core paths. It would be worth adding tests for the parser error cases (ID=0, missing definputdevices, duplicate IDs) to lock in the error UX.

---

Summary: The case-sensitive hash comparison is a real bug that will confuse users. The truncating cast is a latent bug. The live-reload divergence and non-macOS silent-false behavior are worth fixing or clearly mitigating before this is considered production-ready. The rest is design polish.

[claude]

Code Review: definputdevices + (device N) switch condition

Overall this is a well-structured addition with thoughtful design (NonZeroU8, all-of semantics, parse-time ID validation, proper opcode sentinel). The documentation and sample configs are excellent. Below are issues ranging from bugs to style notes.

---

🐛 Bug / Correctness Issues

1. Non-deterministic "last wins" for overlapping device matchers (src/oskbd/macos.rs)

build_device_hash_to_id_map iterates over matchers which is a slice built from a HashMap<NonZeroU8, InputDeviceMatcher>. HashMap iteration order is not stable across runs. If two ID entries both match the same physical device, the winning ID is implementation-defined—not the numerically smaller or larger one, just whichever happens to be iterated last. The warning message says "using {id}" but that ID is unpredictable.

Suggestion: Either make this a hard error at startup (fail-fast with a clear message listing both matching IDs), or explicitly define the semantics as "lowest ID wins" and sort the matchers slice before iterating.

// Sort matchers by ID (ascending) so lowest ID takes precedence on conflict
let mut sorted_matchers: Vec<_> = matchers.to_vec();
sorted_matchers.sort_by_key(|(id, _)| *id);
for (id, matcher) in &sorted_matchers { ... }

---

2. Silent no-op on Linux/Windows when definputdevices is used

parse_definputdevices runs on all platforms. On Linux and Windows, device_id is always None on every KeyEvent, so (device N) conditions always evaluate to false silently. A user who writes a cross-platform config with definputdevices will see completely different behavior on each OS with no warning.

Suggestion: Emit a parse-time warning (or error) when definputdevices is used on non-macOS platforms:

#[cfg(not(target_os = "macos"))]
if input_devices.is_some() {
    log::warn!("definputdevices is only supported on macOS; device conditions will never match on this platform");
}

---

⚠️ Design / Robustness Concerns

3. current_device_id public field with implicit ordering contract (keyberon/src/layout.rs)

pub current_device_id: Option<std::num::NonZeroU8>,

The contract "caller must set this before each event" is implicit and easy to violate. If handle_input_event is ever called without the preceding self.layout.bm().current_device_id = event.device_id line (e.g., in tests, future callers, or after a refactor), the wrong device ID will be used silently—potentially matching the wrong device.

Suggestion: Consider a wrapper method or at minimum strengthen the documentation contract on current_device_id. Even a // INVARIANT: set immediately before handle_input_event comment on both sides would help.

---

4. op2.0 as u8 truncation in opcode_type (keyberon/src/action/switch.rs)

DEVICE_VAL => OpCodeType::Device(
    NonZeroU8::new(op2.0 as u8).expect("device ID must be nonzero"),
),

The as u8 cast silently truncates values 256–65535. While the new_device constructor correctly stores id.get() as u16 (always 1–255), this is a silent assumption. If an opcode stream is ever constructed incorrectly (future bug, fuzz input, etc.), device ID 256 would silently become device ID 0, then .expect() would panic.

Suggestion: Assert or validate the range:

DEVICE_VAL => {
    let raw = op2.0;
    debug_assert!(raw > 0 && raw <= 255, "device ID out of u8 range: {raw}");
    OpCodeType::Device(NonZeroU8::new(raw as u8).expect("device ID must be nonzero"))
}

---

5. Option::is_none_or requires Rust 1.82.0 (src/oskbd/macos.rs)

Option::is_none_or was stabilized in Rust 1.82.0. If this crate has a lower MSRV, this will fail to compile on older toolchains. Worth checking against rust-version in Cargo.toml or CI toolchain pinning.

The equivalent that works on older Rust:

// Instead of: matcher.name.as_ref().is_none_or(|n| ...)
matcher.name.as_ref().map_or(true, |n| ...)

---

🔧 Performance / Hot-Path Concerns

6. self.layout.bm().current_device_id = event.device_id in the hot path (src/kanata/mod.rs:860)

This is called on every single key event. The write itself is trivially cheap (1-byte Option<NonZeroU8>), but the bm() method (presumably borrow_mut()) may involve a runtime borrow check. Given this is described as a hot-path keyboard remapping engine, it's worth verifying bm() is zero-cost or whether the device_id should be threaded through the call stack directly.

If bm() is RefCell::borrow_mut(), consider whether device_id can be passed as a parameter to avoid the implicit mutable state.

---

📝 Test Coverage

7. No parser-level tests for definputdevices.rs

The PR adds switch_device_match and switch_device_opcode_roundtrip tests at the opcode level—these are good. But there are no tests for parse_definputdevices itself. Consider adding cases for:

• Duplicate device IDs → should error
• ID 0 → should error ("device ID must be nonzero")
• Unknown property (serial) → should error
• Empty matcher list → should error
• Missing matcher list for a given ID → should error
• Valid hex vendor/product IDs

---

🎨 Minor Style Notes

8. map_or(false, ...) vs is_ok_and

In build_device_hash_to_id_map:

u16::try_from(kb.vendor_id).map_or(false, |vid| vid == v)

Result::is_ok_and (stable since 1.70) is cleaner:

u16::try_from(kb.vendor_id).is_ok_and(|vid| vid == v)

9. Startup heap allocation for hash comparison

let device_hash = format!("{:x}", kb.hash);
device_hash.eq_ignore_ascii_case(h)

This allocates a String per device per matcher at startup. Not a hot path, but could be avoided with a stack-formatted alternative (e.g., using format_args! with a stack buffer, or comparing via write! to a heapless::String). Low priority since this is startup-only.

10. HashMap::default() → HashMap::new()

Minor: HashMap::new() is the idiomatic form; HashMap::default() works but is less common in Rust code.

---

✅ What's Done Well

• NonZeroU8 for device IDs is the right choice: free None optimization, enforces nonzero at the type level
• Parse-time validation for unknown/duplicate matcher properties
• Sentinel opcode DEVICE_VAL = 855 is consistent with existing LAYER_VAL/BASE_LAYER_VAL pattern
• The docs in config.adoc are thorough with examples, limitations clearly called out
• The comment explaining why input_devices is not updated on live reload is excellent
• All existing 454 tests still passing
• The commented-out device_hash: 0 for output events with explanation is clear

The non-deterministic conflict resolution (#1) and the silent no-op on non-macOS (#2) are the most important issues to address before merge.

[claude]

Code Review: definputdevices + (device N) switch condition

Good overall design. The NonZeroU8 ID type, parse-time validation, and two-opcode sentinel pattern (consistent with LAYER_VAL/BASE_LAYER_VAL) are solid choices. A few issues worth addressing:

---

Bug: Silent truncation in opcode decode

keyberon/src/action/switch.rs — opcode_type():

DEVICE_VAL => OpCodeType::Device(
    NonZeroU8::new(op2.0 as u8).expect("device ID must be nonzero"),
),

op2.0 is a u16. The as u8 cast silently truncates values > 255. Today this is safe because new_device only ever encodes 1–255, but if a future opcode stream ever contains a bad value this produces a wrong (but non-panicking) ID instead of a detectable error. Prefer:

NonZeroU8::new(u8::try_from(op2.0).expect("device ID must fit in u8"))
    .expect("device ID must be nonzero")

---

Silent no-op on Linux / Windows

(device N) evaluates by comparing against event.device_id, which is always None on Linux and Windows — so the condition silently never fires. Users who write a cross-platform config with (device N) will get no error and no output on those platforms.

Consider emitting a parse-time warning (not error) when (device N) appears and the target OS is not macOS, or at minimum document this in the error message when definputdevices is absent. The docs note "macOS only" but a runtime log line would help users debug.

---

#[allow(clippy::too_many_arguments)] accumulating on the hot path

Both actions() and evaluate_boolean() now carry this suppression. These are called on every key event. Adding device_id as a bare parameter is the minimal fix now, but this is the second time this function has grown — it already has 7+ parameters. Before this goes upstream, consider a small context struct:

struct SwitchContext<'a> {
    layers: &'a [u16],
    default_layer: u16,
    device_id: Option<NonZeroU8>,
}

This also makes future additions (e.g. per-window context) less painful.

---

is_none_or MSRV

src/oskbd/macos.rs uses Option::is_none_or, stabilised in Rust 1.82. Please check rust-version in Cargo.toml / the CI matrix — if the project targets anything older, this silently breaks on older toolchains. Alternatively replace with .map_or(true, |x| ...).

---

Missing tests

The two new unit tests (switch_device_match, switch_device_opcode_roundtrip) cover the opcode mechanics well. The gaps:

1. Parser tests for parse_definputdevices: duplicate ID → error; ID = 0 → error; unknown property → error; missing matcher list → error; non-empty but invalid property value.
2. Sim test: end-to-end config with definputdevices + (device N) in a switch, exercised by setting device_id on events in the sim harness. This is the only way to catch wiring bugs (e.g. current_device_id not being set before evaluate_boolean runs).
3. not (device N): verify the negation path works correctly.

---

Minor

• input_devices_vec in macos.rs: clones all InputDeviceMatcher values to pass a &[(id, matcher)] slice. This is startup-only so not a performance issue, but if the matchers were stored as a Vec in Kanata instead of a HashMap, the clone and intermediate Vec could be avoided entirely.
• Conflict resolution in build_device_hash_to_id_map: last-matcher-wins based on iteration order of the config-order slice — this is deterministic (not HashMap-order). The warn! log message on conflict is good. Consider also logging at startup when a matcher produces zero matches (helps users debug misconfigured name/hash matchers).
• Live reload comment: the // Note: input_devices is intentionally not updated on live reload comment is helpful. A log::info! at reload time ("device mappings unchanged; restart kanata to apply new definputdevices") would surface this to users who live-reload and wonder why device routing didn't update.

---

Good groundwork. The two priority items before upstreaming are the as u8 → u8::try_from safety fix and adding at least a parser-level test suite for parse_definputdevices.

[claude]

PR Review: definputdevices + (device N) switch condition

Overall this is a well-structured feature addition. The design decisions (NonZeroU8 IDs, parse-time validation, ALL-of matcher semantics, startup-only device enumeration) are sound. Here are specific findings:

---

Bugs / Correctness Issues

1. Last-match-wins when a device matches multiple IDs (may be surprising)

In build_device_hash_to_id_map (src/oskbd/macos.rs), the outer loop is over matchers and the inner loop is over devices. If two matchers both match the same physical device, the last matcher in the config wins because map.insert unconditionally overwrites. The warning correctly says "using {id}" but users may expect first-match-wins. Consider inverting the loop order (iterate devices outer, matchers inner) and using map.entry(...).or_insert(...) for first-match-wins semantics, which is the less surprising behavior.

2. Dead code: has_device_id! macro is never used

// src/oskbd/mod.rs
macro_rules! has_device_id {
    () => { true }
}

This macro is defined in two #[cfg] variants but never called anywhere in the diff. It can be removed.

---

Hot-Path Performance

The critical path is clean:

• handle_input_event: single field write self.layout.bm().current_device_id = event.device_id() — zero overhead
• evaluate_boolean → OpCodeType::Device(id): single == comparison on Option<NonZeroU8> — optimal (fits in 1 byte, no allocation)
• device_id_for_hash: HashMap lookup called once per event outside the hot loop — acceptable

No concerns here. The design is well-suited to a hot-path engine.

---

API Design

3. current_device_id is pub on Layout, bypassing encapsulation

// keyberon/src/layout.rs
pub current_device_id: Option<std::num::NonZeroU8>,

This works, but exposing the field directly means callers must remember to set it before each event (a fragile contract). A method like set_current_device(id: Option<NonZeroU8>) would make the intent clearer and be harder to misuse. Minor point for a keyberon-internal type.

4. Verbose #[cfg] condition repeated many times

The condition #[cfg(not(all(target_os = "windows", not(feature = "interception_driver"))))] appears 4+ times in oskbd/mod.rs. A helper #[cfg] alias or a feature flag named has_device_id would reduce noise. This is an ergonomics issue, not a bug.

5. parse_definputdevices re-exported as pub unnecessarily

pub use definputdevices::* re-exports parse_definputdevices which is only called internally in parse_cfg_raw_string. Consider making it pub(crate) or pub(super) to narrow the API surface.

---

Test Coverage

6. No integration / sim tests for (device N)

There are unit tests for opcode encoding (switch_device_opcode_roundtrip) and evaluate_boolean (switch_device_match), which are good. But there are no sim_tests exercising the full config-parse → event-dispatch → output path:

;; Would be good to add to sim_tests
(definputdevices
  1 ((name "TestKbd"))
)
(defsrc a)
(deflayer base
  (switch ((device 1)) x break () a break))

Since device_id defaults to None in simulated input, such a test would verify the fallthrough (a) path. Testing with Some(1) would require sim support for injecting device IDs — may be a follow-up.

7. No test for (device N) composed with not/and/or

The boolean operator path through evaluate_boolean is not tested for Device. E.g., (not (device 1)) should evaluate correctly by construction but an explicit test would catch any future regression.

---

Documentation

8. kanata --list referenced but may not exist yet

The definputdevices docs say:

Use kanata --list to see device hashes.

If this flag isn't implemented, this is a documentation error. If it exists pre-PR, great — but worth verifying.

9. Hot-reload limitation is documented but only in two places

The limitation that device mappings aren't updated on live reload is mentioned in docs/config.adoc and in a code comment, but not in the cfg_samples/kanata.kbd example. Users who learn from the sample config might be surprised. Consider adding a comment to the sample.

---

Minor / Nits

10. The device_hash: 0 sentinel for output events

// src/oskbd/macos.rs — From<InputEvent> for DKEvent
device_hash: 0,

Hash 0 theoretically could match a real device (astronomically unlikely but nonzero probability). Adding a comment explaining this is an intentional "no device" sentinel would make the intent clear.

11. parse_hex_or_decimal_u16 error type is Box<dyn std::error::Error>

This is fine for a parser helper but using anyhow::Error or a concrete error type would be more consistent with the rest of the parser codebase.

---

Summary

Category	Finding	Severity
Correctness	Last-match-wins vs first-match-wins for overlapping device matchers	Medium
Dead code	has_device_id! macro unused	Low
API	current_device_id pub field; verbose #[cfg]	Low
Tests	No sim-level integration tests; no not/and/or tests	Medium
Docs	--list flag reference; hot-reload note in sample	Low

The core implementation is solid and the approach is consistent with how LAYER_VAL/BASE_LAYER_VAL work. The last-match-wins behavior (finding #1) and missing integration tests (finding #6) are the most important things to address before merging.

[claude]

Code Review

This is a well-structured PR with a clear design rationale. The NonZeroU8 sentinel choice, parse-time validation of device IDs, and the opcode approach consistent with LAYER_VAL/BASE_LAYER_VAL are all good decisions. A few issues worth addressing before merge:

---

Bug: Non-deterministic winner when a physical device matches multiple configured IDs

In src/oskbd/macos.rs, build_device_hash_to_id_map is called with a slice built from a HashMap:

let input_devices_vec: Option<Vec<_>> = k
    .input_devices
    .as_ref()
    .map(|m| m.iter().map(|(k, v)| (*k, v.clone())).collect());

HashMap iteration order is non-deterministic, so when two matchers both match the same physical device, the warning message "using {id}" is misleading — which ID wins depends on hash map internals, not the user's definition order. If matchers are intended to be non-overlapping, the first match should win and subsequent matches should emit the warning and skip (rather than overwrite). If overlap is intentional (e.g., matching any Apple keyboard to ID 1, then a more specific one to ID 2), the ordering being non-deterministic is a real correctness problem.

Suggestion: Use a Vec<(NonZeroU8, InputDeviceMatcher)> in Cfg/Kanata/KbdIn to preserve definition order, and take the first match wins approach in build_device_hash_to_id_map.

---

No validation that hash strings are valid hex

In definputdevices.rs, the hash matcher value is stored as a raw String with no validation:

"hash" => {
    ...
    matcher.hash = Some(prop_val.to_string());
}

A user typo like (hash "GG:XX:YY") will silently never match any device. Consider validating that the string is a valid hex number at parse time (and possibly normalizing case).

---

evaluate_boolean has 8 parameters

The #[allow(clippy::too_many_arguments)] suppress is a sign this function is getting unwieldy. Consider introducing a SwitchContext<'_> struct to bundle the boolean inputs that don't change across a single event evaluation:

struct SwitchContext<'a> {
    key_codes: ...,
    historical_keys: ...,
    // etc.
    device_id: Option<NonZeroU8>,
}

This would also make future extensions (e.g., a (timestamp ...) condition) easier to add without more #[allow] suppressions and signature churn across callers.

---

current_device_id as a pub field with set-before-use semantics

In keyberon/src/layout.rs, current_device_id is a public mutable field:

pub current_device_id: Option<std::num::NonZeroU8>,

Set in handle_input_event before calling into keyberon:

self.layout.bm().current_device_id = event.device_id();

This works but is easy to forget for future callers/platforms. It also means the field holds stale data between events. Consider instead threading device_id through the keyberon event API — or at minimum adding a doc comment to current_device_id explaining the expected call contract.

---

No integration test for (device N) switch behavior

The unit tests cover opcode roundtrip and evaluate_boolean, but there's no sim test exercising the full stack (parse config → dispatch event with device ID → assert output). The existing sim test framework in src/tests/sim_tests/ could be extended with a helper like:

d:device=1:a   # key-down event from device 1

This would catch regressions that the unit tests cannot (e.g., current_device_id not being set in the event loop).

---

No warning when definputdevices changes during live reload

The comment in kanata/mod.rs is good:

// Note: input_devices is intentionally not updated on live reload.

But users who edit definputdevices and do a live reload will see no indication that the change is silently ignored. Consider emitting a log::warn! if the parsed input_devices differs from the current one during reload.

---

Minor: is_none_or MSRV

Option::is_none_or was stabilized in Rust 1.82.0. Worth confirming this project's MSRV supports it (or using .map_or(true, |n| ...) if not).

---

Minor: parse_hex_or_decimal_u16 return type

The function returns Result<u16, Box<dyn std::error::Error>> which is unusual in this codebase. anyhow::Result or just a bool-returning predicate (since callers always replace the error with a custom message) would be more consistent.

---

Positives

• The NonZeroU8 + Option<NonZeroU8> fitting in 1 byte is a nice memory optimization for KeyEvent
• Duplicate property and duplicate ID errors at parse time are exactly right
• The DEVICE_VAL = 855 sentinel staying consistent with the existing opcode scheme is clean
• #[cfg(not(all(target_os = "windows", not(feature = "interception_driver"))))] correctly omits the field on Windows LLHOOK — good platform-conditional encapsulation
• The two new unit tests cover the critical paths (match, non-match, None device)
• Documentation and sample config are thorough and accurate

[claude]

Code Review: definputdevices + (device N) switch condition

Overall this is well-structured work. The design decisions are sound, the error messages are helpful, and the documentation is thorough. Below are findings ranging from potential bugs to style suggestions.

---

Potential Bug: Silent truncation in opcode_type

keyberon/src/action/switch.rs — OpCode::opcode_type:

DEVICE_VAL => OpCodeType::Device(
    NonZeroU8::new(op2.0 as u8).expect("device ID must be nonzero"),
),

op2.0 is u16; the as u8 silently truncates. While new_device guarantees values ≤ 255 via NonZeroU8, this relies on an implicit invariant. If a corrupted or hand-crafted opcode stream has op2.0 > 255, you get silent truncation rather than a clear error. Prefer u8::try_from(op2.0).expect("device ID must fit in u8") — the same effect but the intent is explicit and the panic message would be clearer.

---

Design Concern: Mutable layout state vs. threaded parameter

keyberon/src/layout.rs — current_device_id field:

pub current_device_id: Option<std::num::NonZeroU8>,

src/kanata/mod.rs:

self.layout.bm().current_device_id = event.device_id();

current_device_id is set on Layout as side-channel state before each event, rather than being passed through the call stack. This works because kanata's event loop is single-threaded, but it creates invisible coupling: callers of Layout::tick / SwitchActions::next must remember to set it first.

The alternative — threading device_id through Switch::actions all the way into evaluate_boolean — is already half-done (the device_id: Option<NonZeroU8> parameter was added to evaluate_boolean and SwitchActions). The current_device_id field in Layout is the point where the two approaches diverge. Consider removing the field and instead passing self.current_device_id (a local variable) directly to switch.actions(...) at the call site in layout.rs. This avoids the invisible pre-event mutation contract.

---

Code Quality: #[allow(clippy::too_many_arguments)]

evaluate_boolean now takes 8 positional arguments. The allow suppression is a code smell that will grow as more conditions are added. Consider a lightweight context struct:

struct EvalCtx<'a> {
    layers: &'a [u16],
    default_layer: u16,
    device_id: Option<NonZeroU8>,
    // ... iterators can stay as generic params on the function
}

This doesn't need to block the PR but is worth a follow-up before the next condition type is added.

---

Missing: No-op on non-macOS platforms is completely silent

docs/config.adoc documents "Currently supported on macOS only", but there's no runtime warning. A user who configures definputdevices on Linux and wonders why (device N) never fires will have no log message to guide them. Suggest adding a startup log::warn! in the Linux/Windows event setup path (or even in the parser) when input_devices.is_some() and the platform isn't macOS:

#[cfg(not(target_os = "macos"))]
if cfg.input_devices.is_some() {
    log::warn!("definputdevices is configured but device matching is only supported on macOS; (device N) conditions will never match");
}

---

Minor: input_devices not updated on live reload — comment placement

src/kanata/mod.rs:

// Note: input_devices is intentionally not updated on live reload.
self.virtual_keys = cfg.fake_keys;

The comment explains why input_devices is not assigned here, but the code line that's missing (self.input_devices = cfg.input_devices;) isn't present. The comment is helpful, but the link to issue #13 would be clearer as // TODO(#13): support hot-reload for definputdevices. The current phrasing could lead a future contributor to add the assignment without reading the referenced issue.

---

Minor: First-match ordering in build_device_hash_to_id_map

src/oskbd/macos.rs — build_device_hash_to_id_map:

The outer loop iterates matchers in definition order. If device D matches both ID 1 and ID 2, the first matcher (by config order) wins. This is reasonable but isn't mentioned in the docs. The log warning does print the conflict, which is good. Consider adding a sentence to the definputdevices docs: "If a device matches multiple entries, the first matching entry wins."

---

Minor: op2.0 as u8 repeated pattern across opcodes

Looking at LAYER_VAL and BASE_LAYER_VAL handling, those use u16 directly as layer indices. The DEVICE_VAL branch casts to u8 — which is correct, but inconsistent formatting vs. the other arms. A short inline comment (// device IDs are 1-255, safe u8 truncation) or the try_from approach above would make this self-documenting.

---

Test Coverage

The two new unit tests (switch_device_match, switch_device_opcode_roundtrip) cover the core evaluation logic well. A few gaps worth tracking:

1. Parser error paths: No test that (device 0) errors, that (device 5) without definputdevices errors, or that duplicate IDs in definputdevices error. The parser has these checks; they'd be easy to test with parse_cfg_raw_string.
2. Integration sim test: A .kbd + sim file that uses (device N) would confirm the end-to-end path (even if device_id is always None in sim, you can test the fallthrough/default case).

---

Nits

• parse_hex_or_decimal_u16 returns Box<dyn std::error::Error> — this is fine for a private parser helper but anyhow::Error or a concrete error type would be more idiomatic in this codebase.
• The definputdevices duplicate-check happens after the first block is already parsed (find + nth(1)). This is correct but slightly asymmetric — the first block is silently accepted even if there's a duplicate. This is fine but worth noting if the error message ever needs to point at both locations.
• is_none_or requires Rust ≥ 1.82. Worth confirming the project's MSRV.

---

Summary: The feature is well-implemented and the documentation is excellent. The two most actionable items before merging are (1) the as u8 truncation in opcode_type and (2) a non-macOS runtime warning to prevent silent misconfiguration. Everything else is polish or follow-up work.

[claude]

Code Review: definputdevices + (device N) switch condition

This is a solid implementation overall. The design choices align well with the rest of the codebase (sentinel opcodes, parse-time validation, NonZeroU8 for compact representation). Below is my feedback organized by priority.

---

Bugs / Correctness

1. expect on hot-path opcode decode

// keyberon/src/action/switch.rs
DEVICE_VAL => OpCodeType::Device(
    NonZeroU8::new(op2.0 as u8).expect("device ID must be nonzero"),
),

This expect is called on every keypress that evaluates a switch condition. The invariant should hold given the parser guarantees, but a corrupted config or an edge case in opcode encoding could trigger a panic at runtime. Consider replacing with an unwrap_or_else(|| log_and_return_false_id) or adding an assertion in debug builds and a safe fallback in release. This is the hot path.

2. Silent miss when no devices match a configured ID

In build_device_hash_to_id_map, if a configured ID matches no devices (e.g., the device name doesn't match, or the device isn't connected), the map entry is simply absent with no diagnostics. A user who misspells a device name will silently get (device N) always returning false. Add a warning after the inner loop:

// After iterating kb_list for a given (id, matcher):
if !map.values().any(|v| v == id) {
    log::warn!("definputdevices: device ID {id} matched no connected devices");
}

---

Design / API

3. evaluate_boolean signature has too many arguments (acknowledged with #[allow])

Both Switch::actions and evaluate_boolean now suppress clippy::too_many_arguments. Adding device_id as a standalone parameter is the seventh+ argument. A SwitchContext struct passed by reference would scale better if another condition type is added in the future:

pub struct SwitchContext {
    pub default_layer: u16,
    pub device_id: Option<NonZeroU8>,
}

This isn't blocking for a first implementation but is worth tracking as tech debt.

4. current_device_id as a pub field on Layout

pub current_device_id: Option<std::num::NonZeroU8>,

This is set by handle_input_event immediately before each event:

self.layout.bm().current_device_id = event.device_id();

Making it pub means any caller can mutate it between when it's set and when switch evaluates, creating a subtle race surface. Consider a set_current_device_id method, or keep it pub(crate). At minimum, the invariant (set per-event, cleared after) should be documented.

5. name matcher is case-sensitive and does substring matching — underdocumented

The docs say "Matches devices whose product name contains the given string" but don't state case sensitivity. contains is case-sensitive in Rust. A user writing (name "apple internal keyboard") will silently fail. Either:

• Document this explicitly: "case-sensitive substring match"
• Or use case-insensitive matching: kb.product_key.to_ascii_lowercase().contains(&n.to_ascii_lowercase())

---

Code Quality

6. Confusing #[cfg] condition on KeyEvent::device_id

#[cfg(not(all(target_os = "windows", not(feature = "interception_driver"))))]
device_id: Option<std::num::NonZeroU8>,

A double negative is hard to read. Consider defining a named cfg alias in a shared location or inverting the logic:

// "llhook" = Windows without interception driver
#[cfg(any(target_os = "macos", target_os = "linux", feature = "interception_driver"))]
device_id: Option<std::num::NonZeroU8>,

7. Redundant case-insensitive comparison after normalizing to lowercase

// build_device_hash_to_id_map
matcher.hash = Some(stripped.to_ascii_lowercase()); // stored as lowercase
// ...
device_hash.eq_ignore_ascii_case(h)  // but format!("{:x}", ...) is already lowercase

Both the stored matcher hash and the formatted hash are lowercase. eq_ignore_ascii_case works but == would be correct and clearer here.

---

Missing Tests

8. No parser-level tests for parse_definputdevices

The switch_device_match and switch_device_opcode_roundtrip unit tests are good. But there are no tests for the config parser covering:

• Duplicate device IDs → error
• Empty matcher list → error
• Unknown matcher property → error
• (device N) without definputdevices block → error
• (device N) with undefined ID → error
• (device N) with ID=0 → error

These are all error paths in parse_definputdevices and parse_switch_case_bool that could regress silently.

---

Minor / Nits

9. parse_hex_or_decimal_u16 returns Box<dyn Error> (parser-only, fine but notable)

This is fine since it's a private parse-time function. Just noting it for consistency — other parse helpers in the codebase may use anyhow directly.

10. input_devices cloned on every macOS event loop setup

let input_devices = k.input_devices.clone();

Vec<(NonZeroU8, InputDeviceMatcher)> contains String fields inside InputDeviceMatcher, so this heap-allocates. This is one-time setup so it's fine, but Arc-wrapping it would avoid the clone if it's ever accessed from multiple threads.

11. live reload note is good, but issue reference may be fragile

// See: https://github.com/malpern/kanata/issues/13

This is an internal fork URL. If this PR is ever submitted upstream (jtroo/kanata), the issue reference will be wrong.

---

Summary

Category	Severity
expect on hot-path decode	Medium — potential panic in release
Silent miss on device match	Medium — bad UX, hard to debug
name case sensitivity undocumented	Medium
current_device_id pub field	Low — encapsulation
Missing parser tests	Low-Medium
cfg double-negative	Low — readability
Redundant eq_ignore_ascii_case	Nit

The architecture is sound and the macOS end-to-end plumbing is clean. Address the hot-path expect and the silent-miss diagnostic before merging, and add at least a few parser error-case tests.

[claude]

Code Review: definputdevices and (device N) switch conditions

Overall this is a well-structured feature addition. The design choices (using NonZeroU8 for IDs, compile-time platform gating, single-pass device matching at startup) are all sound. A few issues worth addressing before merging.

---

Potential Bug: Hash Comparison Without Zero-Padding

File: src/oskbd/macos.rs — build_device_hash_to_id_map

let device_hash = format!("{:x}", kb.hash);
device_hash.eq_ignore_ascii_case(h)

{:x} doesn't zero-pad. If a device hash is 0x0abc, it formats as "abc", not "0abc". If --list outputs the same un-padded format, copy-pasted hashes will always work — but it's a silent foot-gun if they ever diverge. Consider normalizing consistently, or using format!("{:016x}", kb.hash) on both sides.

---

Unnecessary Box<dyn Error> in parse_hex_or_decimal_u16

File: parser/src/cfg/definputdevices.rs

fn parse_hex_or_decimal_u16(s: &str) -> std::result::Result<u16, Box<dyn std::error::Error>> {

The error value is immediately discarded at every call site with .map_err(|_| ...). This allocates for no benefit. Returning Option<u16> or Result<u16, ()> would be cleaner:

fn parse_hex_or_decimal_u16(s: &str) -> Option<u16> {
    let val: u64 = if let Some(hex) = s.strip_prefix("0x").or_else(|| s.strip_prefix("0X")) {
        u64::from_str_radix(hex, 16).ok()?
    } else {
        s.parse().ok()?
    };
    u16::try_from(val).ok()
}

---

Cfg Guard Needs a Comment

File: src/oskbd/mod.rs

#[cfg(not(all(target_os = "windows", not(feature = "interception_driver"))))]
device_id: Option<std::num::NonZeroU8>,

Double-negation cfg guards require readers to reason carefully. This means "present on Linux, macOS, and Windows with Interception; absent on Windows LLHOOK." A short comment stating this directly (as was done in the device_id() doc comment) would make the struct definition self-explanatory. Consider a helper cfg alias like cfg(kanata_device_id) if this pattern recurs.

---

Missing Parser-Level Tests for definputdevices

File: parser/src/cfg/definputdevices.rs

The two new tests cover the opcode/evaluation layer well, but parse_definputdevices itself has no unit tests. Specifically uncovered:

• Duplicate device ID → error
• Empty matcher list → error
• Unknown matcher property → error
• 0 as device ID → error
• vendor_id/product_id hex parsing (e.g. 0x1D50)
• Odd number of top-level items (missing matcher list for last ID)

Since this is a new config block users will write by hand, good parser error coverage is especially valuable.

---

Empty definputdevices Block Is Silently Accepted

File: parser/src/cfg/mod.rs

.map(|expr| parse_definputdevices(expr))

(definputdevices) with no entries would produce Some(vec![]). Any (device N) usage would then fail with "device ID N is not defined," but the empty block itself passes silently. Either emit a warning for an empty block, or treat it as an error — an empty definputdevices is almost certainly a user mistake.

---

Redundant Case-Insensitivity in Hash Comparison

File: src/oskbd/macos.rs

// Storage (already lowercased):
matcher.hash = Some(stripped.to_ascii_lowercase());

// Comparison:
device_hash.eq_ignore_ascii_case(h)

h is already stored as lowercase, so eq_ignore_ascii_case is redundant. Either use == in the comparison or drop the to_ascii_lowercase() at storage time. Minor, but signals a slight inconsistency in the normalization strategy.

---

Minor Positives Worth Calling Out

• NonZeroU8 for device IDs is exactly the right type — it encodes the "unset" state without an extra boolean and prevents ID 0 misuse.
• The has_device_filter fix (preventing fallback to all-devices when an include/exclude list matches nothing) is a correct behavior improvement over the old code.
• The Karabiner virtual device warning in list_devices_macos is thoughtful UX.
• Documenting the live-reload non-update behavior inline (// Note: input_devices is intentionally not updated on live reload) is good practice.
• Error messages throughout parse_definputdevices are specific and actionable.

---

Summary

Severity	Issue
Bug (minor)	Hash format not zero-padded — may silently fail if --list output diverges
Design	Box<dyn Error> in parse_hex_or_decimal_u16 allocates needlessly
Readability	Double-negation cfg guard needs a clarifying comment
Test coverage	No unit tests for parse_definputdevices parser
UX	Empty definputdevices block should warn or error
Nit	Redundant eq_ignore_ascii_case on pre-normalized string

The core design and implementation are solid. The hash formatting issue is the only one I'd call a real bug risk — the rest are polish and maintainability.