feat(moza-cli): harden moza protocol and cli verification tools

[EffortlessSteven]

Moza Stack and CLI Hardening Audit

This PR hardens the Moza Racing R5 driver stack and the CLI verification tools for real-time safety and stability.

Key Changes

• Moza Stack: Added strict unwrap() denials and ADR citations (ADR-0006, ADR-0007) to hbp, srp, moza-wheelbase-report, and hid-moza-protocol.
• CLI Tools: Refactored yaml_sync_check, validate_adr, and generate_docs_index to use anyhow error handling and LazyLock for regex.
• Verification: Verified with 83 passing unit tests (60 protocol, 23 CLI).
• Compliance: All modified components now follow project safety lints and documentation standards.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:744 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/linux.rs:1253 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/linux.rs:1282 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/mod.rs:203 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/compat_impl.rs:61 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:217 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:744 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/ui/src/commands.rs:357 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/linux.rs:1253 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/linux.rs:1282 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/mod.rs:203 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/compat_impl.rs:61 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[coderabbitai]

Actionable comments posted: 36

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (14)

crates/ks/src/lib.rs (1)

67-90: ⚠️ Potential issue | 🟠 Major

Don't silently reinterpret unsigned axis bindings as signed encoders.

The new helpers make signedness explicit, but parse() still writes every encoder and rotary binding through parse_i16(). A source declared with signed: false will therefore wrap values above i16::MAX into negatives instead of failing or being converted deliberately. Please gate these slots on axis.signed, or reject unsigned bindings here so bad maps do not turn into corrupted encoder data.

Also applies to: 329-339

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/ks/src/lib.rs` around lines 67 - 90, KsAxisSource's parsing currently
always uses parse_i16 causing unsigned bindings to be misinterpreted; update the
consumer (the parse() callsite that iterates encoder and rotary bindings) to
check the KsAxisSource.signed flag and call parse_u16 when signed is false (or
return an error/None for unsigned where appropriate), or alternatively reject
unsigned bindings before calling parse_i16. Locate usages of
KsAxisSource::parse_i16/parse_u16 (and the parse_bytes helper) and gate them on
self.signed so unsigned sources use parse_u16 (and are converted/handled
deliberately) while signed sources use parse_i16 to avoid wrapping high unsigned
values into negatives.

crates/schemas/build.rs (1)

67-71: ⚠️ Potential issue | 🟠 Major

Remove the no-op stub function and clarify the fallback behavior.

The create_protobuf_stubs() function on line 67-71 does nothing but return Ok(()), yet is called when protoc compilation fails. The comment claims it's "no longer needed" but kept "for backward compatibility"—but this is misleading.

The actual fallback is the committed crates/schemas/src/generated/wheel.v1.rs file in git, which allows builds to succeed even without protoc. The create_protobuf_stubs() call is therefore dead code that confuses readers about how the build handles missing protoc.

Fix: Either remove the function and its call on line 28, or replace the error handling to explicitly document that the committed generated files serve as the fallback.

Also update the error message on line 19—saying "Protobuf functionality will be disabled" is misleading when the build will actually continue to work using the committed generated files.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/schemas/build.rs` around lines 67 - 71, The create_protobuf_stubs()
function is a no-op and its call is dead code that misleads readers about the
fallback; remove the create_protobuf_stubs() function and any call sites (search
for create_protobuf_stubs()) so the code relies on the committed generated file
crates/schemas/src/generated/wheel.v1.rs as the real fallback, and update the
error message that currently says "Protobuf functionality will be disabled"
(search for that exact string) to clearly state that protoc failed but the build
will continue using the committed generated protobuf sources as a fallback.

crates/hbp/src/lib.rs (1)

447-454: ⚠️ Potential issue | 🟡 Minor

Replace .expect() with pattern matching in proptest tests.

Lines 451, 466, 481, 493 use .expect("just checked is_some") which violates the coding guidelines prohibiting unwrap() and expect() in test code. The #![deny(clippy::unwrap_used)] lint at crate level will also flag these.

Proposed fix using if-let pattern

         #[test]
         fn prop_two_byte_report_always_parses(lo in 0u8..=255u8, hi in 0u8..=255u8) {
             let report = [lo, hi];
             let parsed = parse_hbp_usb_report_best_effort(&report);
             prop_assert!(parsed.is_some());
-            let sample = parsed.expect("just checked is_some");
-            prop_assert_eq!(sample.handbrake, u16::from_le_bytes([lo, hi]));
-            prop_assert_eq!(sample.button_byte, None);
+            if let Some(sample) = parsed {
+                prop_assert_eq!(sample.handbrake, u16::from_le_bytes([lo, hi]));
+                prop_assert_eq!(sample.button_byte, None);
+            }
         }

Apply similar refactoring to prop_four_byte_nonzero_id_uses_prefixed_layout (line 466), prop_three_byte_zero_first_uses_raw_layout (line 481), and prop_raw_encoding_round_trip (line 493).

As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests (e.g., #[test] fn foo() -> Result<(), Error>), explicit assertions (assert!(result.is_ok()), assert_eq!), or test helper macros"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/hbp/src/lib.rs` around lines 447 - 454, The tests
(prop_two_byte_report_always_parses,
prop_four_byte_nonzero_id_uses_prefixed_layout,
prop_three_byte_zero_first_uses_raw_layout, prop_raw_encoding_round_trip) call
.expect("just checked is_some") on the result of
parse_hbp_usb_report_best_effort; replace those .expect() calls with pattern
matching (e.g., after prop_assert!(parsed.is_some()) use if let Some(sample) =
parsed { ... } else { prop_assert!(false, "parsed was None") }) so you
destructure the Some value without using unwrap/expect and then perform the
existing prop_assert_eq! checks on sample. Ensure each test uses the same if-let
pattern so the crate-level clippy lint for unwrap_used is satisfied.

crates/engine/src/two_phase_apply.rs (1)

495-525: ⚠️ Potential issue | 🟠 Major

Remove the remaining unwrap() calls before claiming this test module is hardened.

The touched tests at Lines 516-517, Line 575, and Lines 728-729 still use unwrap(), so this file still depends on #[allow(clippy::unwrap_used)] at Line 353. Convert those awaits to ?/explicit assertions like the other updated tests and then drop the allow.

♻️ Minimal fix

-#[cfg(test)]
-#[allow(clippy::unwrap_used)]
-mod tests {
+#[cfg(test)]
+mod tests {
@@
-        let result1 = result_rx1.await.unwrap();
-        let result2 = result_rx2.await.unwrap();
+        let result1 = result_rx1.await?;
+        let result2 = result_rx2.await?;
@@
-        let result = result_rx.await.unwrap();
+        let result = result_rx.await?;
@@
-        let result1 = result_rx1.await.unwrap();
-        let result2 = result_rx2.await.unwrap();
+        let result1 = result_rx1.await?;
+        let result2 = result_rx2.await?;

As per coding guidelines, **/*.rs: Test code must not use unwrap() or expect(). Prefer Result-returning tests (e.g., #[test] fn foo() -> Result<(), Error>), explicit assertions (assert!(result.is_ok()), assert_eq!), or test helper macros.

Also applies to: 553-584, 703-739

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/two_phase_apply.rs` around lines 495 - 525, The test
test_two_phase_apply_multiple_pending currently uses unwrap() on channel awaits
(result_rx1.await.unwrap() and result_rx2.await.unwrap()) which keeps the file
depending on #[allow(clippy::unwrap_used)]; change those awaits to propagate
errors or assert on their Result instead (e.g., let result1 = result_rx1.await?
or assert!(result_rx1.await.is_ok()) and then unwrap the success fields safely),
update similarly any other tests referenced (the ones around the other mentioned
ranges that call .await.unwrap()), and then remove the file-level
#[allow(clippy::unwrap_used)] so the module no longer uses unwrap/expect in
tests; reference the functions/values result_rx1, result_rx2, and
test_two_phase_apply_multiple_pending when making the edits.

crates/openracing-errors/tests/comprehensive_tests.rs (2)

536-555: ⚠️ Potential issue | 🟡 Minor

Missing RTError::AccessViolation in rt_error_into_openracing_error test.

The all_rt array doesn't include AccessViolation, so its From impl conversion to OpenRacingError isn't tested here.

🔧 Proposed fix to add the missing variant

         RTError::BufferOverflow,
         RTError::DeadlineMissed,
         RTError::ResourceUnavailable,
+        RTError::AccessViolation,
     ];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/openracing-errors/tests/comprehensive_tests.rs` around lines 536 -
555, The test's all_rt array is missing RTError::AccessViolation so the
From<RTError> for OpenRacingError conversion isn't covered; update the test
(rt_error_into_openracing_error) by adding RTError::AccessViolation to the
all_rt array alongside the other RTError variants, then ensure the loop still
converts each variant via let ore: OpenRacingError = (*rt).into(); and asserts
category() == ErrorCategory::RT and that ore.to_string() contains
rt.to_string().

---

23-42: ⚠️ Potential issue | 🟡 Minor

Missing RTError::AccessViolation in the variants list.

The all_variants_have_nonempty_display test does not include the new AccessViolation variant, which means its Display implementation won't be verified here.

🔧 Proposed fix to add the missing variant

         RTError::BufferOverflow,
         RTError::DeadlineMissed,
         RTError::ResourceUnavailable,
+        RTError::AccessViolation,
     ];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/openracing-errors/tests/comprehensive_tests.rs` around lines 23 - 42,
The test all_variants_have_nonempty_display is missing the new
RTError::AccessViolation variant, so update the variants array in that test to
include RTError::AccessViolation (alongside the existing
RTError::DeviceDisconnected, RTError::TorqueLimit, etc.) so the loop that calls
v.to_string() verifies the Display text for AccessViolation as well.

crates/hid-vrs-protocol/src/output.rs (1)

409-417: 🧹 Nitpick | 🔵 Trivial

Test function should return Result per coding guidelines.

The test_torque_to_magnitude test uses assertions without returning a Result type, which is inconsistent with the other tests in this file and violates the coding guideline that test code should use Result-returning tests.

🔧 Proposed fix to use Result-returning pattern

     #[test]
-    fn test_torque_to_magnitude() {
+    fn test_torque_to_magnitude() -> Result<(), Box<dyn std::error::Error>> {
         assert_eq!(torque_to_magnitude(0.0, 10.0), 0);
         assert_eq!(torque_to_magnitude(5.0, 10.0), 5000);
         assert_eq!(torque_to_magnitude(10.0, 10.0), 10000);
         assert_eq!(torque_to_magnitude(-10.0, 10.0), -10000);
         assert_eq!(torque_to_magnitude(20.0, 10.0), 10000);
         assert_eq!(torque_to_magnitude(-20.0, 10.0), -10000);
+        Ok(())
     }

As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests (e.g., #[test] fn foo() -> Result<(), Error>), explicit assertions (assert!(result.is_ok()), assert_eq!), or test helper macros"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/hid-vrs-protocol/src/output.rs` around lines 409 - 417, Change the
test_torque_to_magnitude unit test to be a Result-returning test to match
project guidelines: update the function signature to return Result<(), Box<dyn
std::error::Error>> (or the same Result type used by other tests in this file),
keep the existing assert_eq! calls, and add a final Ok(()) return; this uses the
existing torque_to_magnitude function name to locate the test and ensures the
test follows the Result-returning pattern used elsewhere.

crates/engine/src/safety/integration.rs (1)

846-847: 🧹 Nitpick | 🔵 Trivial

#[allow(clippy::unwrap_used)] contradicts coding guidelines.

The test module explicitly allows unwrap_used, but the coding guidelines state: "Test code must not use unwrap() or expect(). Prefer Result-returning tests or explicit assertions."

Consider refactoring tests to use Result-returning test functions or explicit assertions instead of blanket-allowing unwrap.

As per coding guidelines: "Avoid unwrap() and expect() in test code; use Result-returning tests or explicit assertions instead."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/safety/integration.rs` around lines 846 - 847, Remove the
blanket #[allow(clippy::unwrap_used)] on mod tests and refactor the tests inside
the tests module to avoid unwrap()/expect(): convert failing tests to return
Result<(), E> and use the ? operator for fallible calls or replace
unwrap()/expect() with explicit assertions (e.g., assert_eq!,
assert!(matches!(...)), or map_err to propagate errors). Locate the tests under
mod tests in this file and for each test that calls unwrap()/expect(), change
its signature to return Result<(), Box<dyn std::error::Error>> (or another
concrete error type) and replace unwrap()/expect() with ? or explicit assertion
checks so the clippy allow attribute can be removed. Ensure all tests compile
and run as Result-returning tests or use explicit assertions before deleting the
#[allow(clippy::unwrap_used)] attribute.

crates/schemas/src/ipc_conversion.rs (1)

564-577: 🧹 Nitpick | 🔵 Trivial

Test helper functions use panic!, which contradicts the guideline spirit.

The must and must_some helpers use panic! on failure. While not literally unwrap() or expect(), this has the same effect of panicking in tests rather than using proper assertions or Result-returning tests.

As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests (e.g., #[test] fn foo() -> Result<(), Error>), explicit assertions (assert!(result.is_ok()), assert_eq!), or test helper macros."

Consider refactoring tests to return Result and propagate errors, or use assertions that provide better diagnostics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/schemas/src/ipc_conversion.rs` around lines 564 - 577, The helpers
must and must_some currently call panic! on error which violates the no-panic
test guideline; change them to return Results (e.g., fn must<T, E:
std::fmt::Debug>(r: Result<T,E>) -> Result<T, Box<dyn std::error::Error>> and fn
must_some<T>(o: Option<T>, msg: &str) -> Result<T, Box<dyn std::error::Error>>)
or to use assertions in tests that call them; update all callers in tests to
propagate errors by making tests return Result<(), Box<dyn Error>> and using ?
to propagate failures, or replace calls with explicit assert!(r.is_ok()) /
assert_eq!(...) so failures produce test assertions rather than panics (refer to
functions named must and must_some when locating the helpers and their call
sites).

crates/engine/src/pipeline.rs (1)

848-861: 🧹 Nitpick | 🔵 Trivial

Test code uses unwrap() and panic-based helpers, inconsistent with PR pattern.

This test module uses #[allow(clippy::unwrap_used)] and a panic-based must(...) helper, which contradicts the coding guideline: "Test code must not use unwrap() or expect(). Prefer Result-returning tests."

Other files in this PR (ports.rs, standalone.rs) were refactored to use Result-returning tests. This file should follow the same pattern for consistency.

♻️ Suggested refactor for test helper

 #[cfg(test)]
-#[allow(clippy::unwrap_used)]
 mod tests {
     use super::*;
 
     use crate::filters::*;
 
-    #[track_caller]
-    fn must<T, E: std::fmt::Debug>(r: Result<T, E>) -> T {
-        match r {
-            Ok(v) => v,
-            Err(e) => panic!("unexpected Err: {e:?}"),
-        }
-    }
+    // Convert tests to return Result<(), Box<dyn std::error::Error>>
+    // and use `?` operator instead of must() helper

As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/pipeline.rs` around lines 848 - 861, The tests module
currently suppresses unwrap warnings and defines a panic-based helper must(...)
which contradicts our Result-returning test pattern; remove
#[allow(clippy::unwrap_used)] and replace panic-based must with either using the
? operator directly in each test (change tests to return Result<(), Box<dyn
std::error::Error>>) or implement a small helper like try_get<T, E>(r:
Result<T,E>) -> Result<T,E> and propagate errors; update any test functions
referencing must to return Result and use ? (or try_get) so no
unwrap/expect/panic helpers remain—look for the tests module and the must
function to update.

crates/openracing-native-plugin/src/spsc.rs (1)

283-339: ⚠️ Potential issue | 🟡 Minor

Tests use expect() violating coding guidelines.

The tests in this file use expect() extensively (lines 286, 294, 300, 303, 312, 320, 327, 334), which violates the coding guideline: "Test code must not use unwrap() or expect(). Prefer Result-returning tests."

♻️ Suggested refactoring for one test

 #[test]
-fn test_spsc_channel_creation() {
-    let frame_size = 64;
-    let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
-    assert!(channel.frame_size() == frame_size);
-    assert!(!channel.is_shutdown());
-}
+fn test_spsc_channel_creation() -> Result<(), NativePluginError> {
+    let frame_size = 64;
+    let channel = SpscChannel::new(frame_size)?;
+    assert_eq!(channel.frame_size(), frame_size);
+    assert!(!channel.is_shutdown());
+    Ok(())
+}

Apply similar refactoring to test_spsc_write_read, test_spsc_ring_buffer, and test_shutdown_flag.

As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/openracing-native-plugin/src/spsc.rs` around lines 283 - 339, Replace
panic-causing expect() calls in the tests by converting each test function to
return Result<(), Box<dyn std::error::Error>> and using the ? operator for
fallible calls; specifically update test_spsc_channel_creation,
test_spsc_write_read, test_spsc_ring_buffer, and test_shutdown_flag to return
Result and replace .expect("...") with ? on SpscChannel::new,
SpscChannel::with_capacity, writer.write, and reader.read, keeping the same
assertions (e.g., assert_eq!, assert!, etc.) and call Ok(()) at the end of each
test.

crates/engine/src/engine.rs (2)

1202-1224: ⚠️ Potential issue | 🟡 Minor

Replace .expect() calls in test helpers.

Lines 1203 and 1217 use .expect() for DeviceId parsing.

Proposed fix for create_capturing_device and create_test_config

-fn create_capturing_device(writes: Option<Arc<Mutex<Vec<f32>>>>) -> Box<dyn HidDevice> {
-    let device_id = "test-device".parse::<DeviceId>().expect("valid device id");
-    Box::new(CapturingDevice::new(device_id, writes))
+fn create_capturing_device(writes: Option<Arc<Mutex<Vec<f32>>>>) -> Result<Box<dyn HidDevice>> {
+    let device_id = "test-device".parse::<DeviceId>()?;
+    Ok(Box::new(CapturingDevice::new(device_id, writes)?))
 }

 fn create_test_config() -> EngineConfig {
     EngineConfig {
-        device_id: "test-device".parse::<DeviceId>().expect("valid device id"),
+        device_id: "test-device".parse::<DeviceId>().unwrap_or_else(|_| {
+            // This is a test constant that cannot fail
+            panic!("test device ID parsing failed")
+        }),

Or better, make create_test_config return Result<EngineConfig>.

As per coding guidelines: "Test code must not use unwrap() or expect()."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/engine.rs` around lines 1202 - 1224, Tests currently call
.expect() when parsing DeviceId in create_capturing_device and
create_test_config; replace these panics by handling the parse Result: either
propagate the error by changing create_test_config to return
Result<EngineConfig, DeviceIdParseError> (or appropriate error type) and map the
parsed device_id into the config, and change create_capturing_device to return
Result<Box<dyn HidDevice>, _> (or accept a DeviceId argument) so callers can
handle failures; alternatively, provide a safe test-only constant DeviceId
created via DeviceId::from_str(...).ok_or(...)? and propagate errors instead of
using expect() in both create_capturing_device and create_test_config.

---

1119-1128: ⚠️ Potential issue | 🟡 Minor

Replace .expect() with error propagation in CapturingDevice::new.

Line 1125 uses .expect("valid torque") which violates coding guidelines.

Proposed fix

 impl CapturingDevice {
-    fn new(device_id: DeviceId, writes: Option<Arc<Mutex<Vec<f32>>>>) -> Self {
+    fn new(device_id: DeviceId, writes: Option<Arc<Mutex<Vec<f32>>>>) -> Result<Self> {
         let capabilities = DeviceCapabilities::new(
             false,
             true,
             true,
             false,
-            TorqueNm::new(25.0).expect("valid torque"),
+            TorqueNm::new(25.0)?,
             10000,
             1000,
         );
         // ... rest unchanged ...
-        Self { info, capabilities, writes, connected: true }
+        Ok(Self { info, capabilities, writes, connected: true })
     }
 }

As per coding guidelines: "Test code must not use unwrap() or expect()."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/engine.rs` around lines 1119 - 1128, The constructor
CapturingDevice::new currently uses TorqueNm::new(...).expect("valid torque");
change CapturingDevice::new to return Result<Self, E> (choose the crate's
existing error type or anyhow::Error) so you can propagate the
TorqueNm::new(...) failure with the ? operator instead of calling expect; update
the DeviceCapabilities::new call to use the result of TorqueNm::new(...)?,
adjust all callers of CapturingDevice::new to handle the Result (propagate or
map the error), and ensure any test helpers are updated accordingly.

crates/engine/src/hid/mod.rs (1)

272-288: 🧹 Nitpick | 🔵 Trivial

Remove stale #[allow(clippy::expect_used)] attribute.

Line 272 allows clippy::expect_used but the expect() call was replaced with unsafe { TorqueNm::new_unchecked(...) }. The allow attribute is no longer needed.

Proposed fix

-    #[allow(clippy::expect_used)]
     pub fn to_device_capabilities(&self) -> DeviceCapabilities {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/hid/mod.rs` around lines 272 - 288, The
#[allow(clippy::expect_used)] attribute above the to_device_capabilities
function is stale and should be removed; edit the pub fn
to_device_capabilities(&self) -> DeviceCapabilities implementation to delete the
#[allow(clippy::expect_used)] line (and ensure no other expect() calls remain in
to_device_capabilities), leaving the function body unchanged (including the
unsafe TorqueNm::new_unchecked(clamped_nm) usage).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@check_errors_v5.txt`:
- Around line 1-903: The Windows HID file (windows.rs) fails to compile due to
missing windows crate APIs, misplaced imports, and a missing Seqlock; fix by
enabling the needed windows crate features in Cargo.toml (add Win32::System::IO
for WriteFile, Win32::System::WindowsProgramming for AvSet/AvRevert*,
Win32::UI::Pnp, etc.), or wrap the entire module with #[cfg(target_os =
"windows")] and provide a non-Windows stub; update imports in
crates::engine::src::hid::windows.rs to reference the correct public re-exports
(replace crate::input::{MozaInputState, TelemetryData} with the proper paths
such as crate::hid::MozaInputState or racing_wheel_schemas::... and fix vendor
imports like encode_torque_report_for_device/DeviceTelemetryReport to their
actual module or re-export), and either implement or import Seqlock (or remove
its usage) so Seqlock is available to the module; after enabling windows
features and correcting import paths (e.g., GetOverlappedResult, OVERLAPPED,
WriteFile, AvSetMmThreadCharacteristicsW, AvRevertMmThreadCharacteristics, Pnp
items, DeviceEvent/DeviceHealthStatus/HidDevice names) re-run cargo build to
ensure errors are resolved.

In `@check_output_v10.txt`:
- Around line 1-18: Remove the committed build log artifacts (e.g.,
check_output_v10.txt and all similar files like check_output_v2..v19.txt and
check_errors_v3..v9.txt) from the repository and add appropriate ignore patterns
to .gitignore so they aren't re-added; remove them from git history/staging with
git rm (or git rm --cached) and commit the deletions, then add patterns such as
check_output*.txt and check_errors*.txt to .gitignore to prevent future commits.
Ensure you only delete/generated log files and not source files—verify filenames
before removing and commit the .gitignore change together with the removals.

In `@check_output_v14.txt`:
- Around line 1-35: Committed build log check_output_v14.txt should be removed
and ignored; delete the file from the PR and add a gitignore entry. Remove the
file from the repository (git rm check_output_v14.txt or git rm --cached if you
want to keep locally), add a pattern like check_output*.txt to .gitignore,
commit the .gitignore change and the removal, and push; if these logs are
intentionally kept, move check_output_v14.txt into a documented location
(docs/ci or diagnostics/) and update README explaining purpose instead of
keeping it at repo root.

In `@check_output_v16.txt`:
- Around line 1-26: The code is importing RegisterDeviceNotificationW from the
wrong module and keeping an unused AvRevertMmThreadCharacteristics import;
update the import block in windows.rs so RegisterDeviceNotificationW is imported
from windows::Win32::UI::WindowsAndMessaging (not
windows::Win32::Devices::DeviceAndDriverInstallation) and remove
AvRevertMmThreadCharacteristics from the imported symbols (keep
AvSetMmThreadCharacteristicsW if still needed); ensure the import list
references the correct module and only the used symbols (e.g.,
RegisterDeviceNotificationW, AvSetMmThreadCharacteristicsW).

In `@check_output_v2.txt`:
- Around line 1-40: The committed build log contains warnings about unused code;
remove the log file from version control and fix the source warnings: either use
or mark as intentionally unused the tuple fields in SendableHdevnotify and
SendableHwnd (the structs named SendableHdevnotify(HDEVNOTIFY) and
SendableHwnd(HWND)) by adding #[allow(dead_code)] with a comment if they’re
reserved, and either remove DeviceInputs::from_moza_input_state or annotate it
with #[allow(dead_code)] and a short comment explaining its future use so the
compiler warnings are resolved.

In `@crates/compat/Cargo.toml`:
- Around line 13-15: The manifest comment about openracing-hid-common is
incorrect because openracing-hid-common is now a runtime dependency; update the
Cargo.toml comment that currently reads "Only used in test builds - no runtime
dependencies" by either removing it or rewriting it to reflect that
openracing-hid-common is a runtime dependency (mentioning the crate name
openracing-hid-common so you can locate the entry) and, if useful, add a brief
accurate note about its runtime role.

In `@crates/engine/src/device.rs`:
- Around line 202-212: The torque variance loop can read an uninitialized buffer
slot when using pairs—ensure you only iterate over valid consecutive pairs by
computing a pair_count = state.torque_history_len.min(100).saturating_sub(1) (or
return/skip if pair_count == 0), iterate i in 0..pair_count when building
sum_diff, and divide by pair_count as f32; update the logic that sets
state.hands_on to use this corrected torque_variance computation and still
reference state.torque_history, state.torque_history_idx, and
state.torque_history_len.

In `@crates/engine/src/engine.rs`:
- Around line 1105-1109: The helper create_test_device currently calls .expect()
on "test-device".parse::<DeviceId>(); change create_test_device to return a
Result (e.g., Result<Box<dyn HidDevice>, E> using your crate error type or
anyhow::Error), perform the parse with ? to propagate parse errors, construct
the VirtualDevice and return Box::new(virtual_device) wrapped in Ok(...), and
update all test callers of create_test_device to use the ? operator (e.g., let
device = create_test_device()?); this removes the .expect() usage while keeping
DeviceId and VirtualDevice code paths intact.

In `@crates/engine/src/hid/mod.rs`:
- Around line 18-19: The project makes Seqlock an alias only on Linux which
breaks windows.rs (it imports super::Seqlock); add a cross-platform stub or
platform-guard so Windows compiles: define Seqlock (aliasing
crate::input::SnapshotMailbox) behind a cfg that provides a non-Linux fallback
type or stub in the same module as the #[cfg(target_os = "linux")] alias, or
update windows.rs to cfg-gate uses of Seqlock; locate the Seqlock alias in
mod.rs and usages in windows.rs and either (A) add a non-Linux stub type named
Seqlock matching the API used by windows.rs or (B) wrap windows.rs code that
references Seqlock with appropriate #[cfg(target_os = "linux")] to avoid the
missing symbol on Windows.

In `@crates/engine/src/protocol.rs`:
- Around line 3-5: The module-level doc comment currently links ADR-0003 using a
local file URI (file:///h:/Code/...), which leaks a machine path into rustdoc;
update the crate/engine/src/protocol.rs module doc (the top ///! comment
referencing ADR-0003) to use a repo-stable reference instead — for example
replace the file:///... link with a relative or canonical repo URL (or just the
ADR identifier link like [ADR-0003]: ./docs/adr/0003-owp1-protocol.md or a
GitHub raw/blob URL) so the ADR citation resolves for all users and no local
drive paths are exposed.

In `@crates/engine/src/safety.rs`:
- Around line 2-6: The ADR link in the module doc comment currently uses a
non-portable local file URI
(file:///h:/Code/Rust/OpenRacing/docs/adr/0006-safety-interlocks.md); update the
link in the crate root comment (the ADR-0006 reference in
crates/engine/src/safety.rs) to a portable path—either a relative path within
the repository (e.g., ../docs/adr/0006-safety-interlocks.md) or the canonical
repo URL to the ADR file on your remote (GitHub/GitLab) so other developers can
resolve it; ensure the link text "[ADR-0006]" remains unchanged.

In `@crates/engine/src/safety/integration.rs`:
- Around line 255-260: The current check uses
self.watchdog_system.has_registered_components() to unconditionally push
FaultType::SafetyInterlockViolation, which causes false positives during normal
startup; update the logic around that check in integration.rs so the interlock
is only triggered when components are actually expected—e.g., add and check a
startup grace period (timestamp like self.startup_time or uptime), or an
explicit startup_complete / expect_components flag from configuration
(self.config.expect_components or self.startup_complete) before pushing into
result.new_faults; keep the existing references to
self.watchdog_system.has_registered_components() and
result.new_faults.push(FaultType::SafetyInterlockViolation) but wrap them in the
new conditional that verifies either the grace period has elapsed or
startup_complete/expect_components is true.

In `@crates/hid-moza-protocol/src/lib.rs`:
- Around line 10-12: The crate-level ADR-0007 documentation is currently using
outer doc comments (///) immediately above `pub mod direct`, so it will attach
to the `direct` module instead of the crate; change those comments to inner doc
comments (`//!`) and place them at the top of the file (or move them into the
crate-level docstring) so the ADR-0007 text becomes crate-level documentation
rather than module-level documentation for `pub mod direct`.

In `@crates/hid-moza-protocol/src/protocol.rs`:
- Around line 4-9: The ADR link in the module docs (top of protocol.rs) uses an
absolute local file URL; replace it with a repository-relative path so others
can resolve it (e.g. change the reference label [ADR-0006] to point at
"docs/adr/0006-safety-interlocks.md" or "../docs/adr/0006-safety-interlocks.md"
or use a conventional docs reference style without file:///). Update the
reference near the module doc comment where [ADR-0006] is defined so the link is
relative to the repo instead of "file:///h:/Code/Rust/OpenRacing/…".

In `@crates/hid-simagic-protocol/src/lib.rs`:
- Line 52: Remove the crate-wide #![allow(missing_docs)] in lib.rs and instead
apply #[allow(missing_docs)] only to the smallest public items or modules that
currently lack docs (e.g., annotate specific module declarations or individual
pub structs/enums/functions that are undocumented); update lib.rs to delete the
top-level attribute and add scoped #[allow(missing_docs)] attributes on the
exact module or item identifiers (module names or pub type/function names) that
still need follow-up documentation so the rest of the public API remains checked
for missing docs.

In `@crates/integration-tests/src/acceptance.rs`:
- Line 550: The test currently calls fault_response_limit() and assigns it to
_response_limit but still hardcodes "50ms" in the verification and error
message; change the verification and any error/expect messages that mention
"50ms" to use the computed _response_limit (or rename to response_limit if you
prefer) so the test honors CI-configured limits; update both occurrences (around
the verification at the first instance and the similar block at the second
occurrence) to reference response_limit/fault_response_limit() instead of the
literal 50ms.

In `@crates/integration-tests/src/common.rs`:
- Around line 69-82: The code currently allocates by collecting the filtered
entries into relevant_history inside verify_torque_ramp_to_zero; instead iterate
without allocation by replacing the collect+last logic with a reverse search
over torque_history, e.g. use self.torque_history.iter().rev().find(|(t, _)| *t
>= start_time) (or Iterator::rfind) to get the last matching entry and then
apply the same check on the found (end_time, last_torque) tuple; update the
map/unwrap_or(false) usage accordingly so no Vec is created.
- Around line 56-67: The torque_history VecDeque is created with
VecDeque::with_capacity(1000) but set_torque enforces a max of 10000, causing
repeated reallocations; update the initialization to match the intended cap or
reduce the cap to the initial capacity. Locate torque_history initialization
(VecDeque::with_capacity) and either change the capacity to 10000 or change the
cap in set_torque from 10000 to 1000; ensure semantics around
last_torque_command, push_back and pop_front remain unchanged and add a brief
comment documenting the chosen limit.

In `@crates/ks/src/lib.rs`:
- Around line 252-255: KsReportMap::empty() leaves clutch_mode_hint as Unknown
causing parse()/snapshot to keep Unknown even when clutch bindings exist; update
the code that produces the snapshot (the parse() path that copies
clutch_mode_hint into the snapshot) to derive a fallback clutch_mode when
clutch_mode_hint == Unknown: if clutch_combined_axis.is_some() set mode to
Combined; else if both clutch_left_axis.is_some() and
clutch_right_axis.is_some() set mode to Separate/Both; else if only one of
clutch_left_axis or clutch_right_axis is set choose Left or Right accordingly;
ensure this logic is applied where clutch_mode_hint is copied (parse()/snapshot
creation and the other similar spots referenced around the same area) so
both_clutches_pressed() will use the derived mode.

In `@crates/openracing-hid-common/src/math.rs`:
- Around line 23-24: The assertion for safe_clamp in math.rs contains leftover
debugging/working notes in the comment after assert_eq!(safe_clamp(f32::NAN,
-10.0, -5.0), -5.0) — remove the informal "wait" and calculation trace and
either replace with a concise, purposeful comment explaining the expected
behavior for NaN inputs in safe_clamp or delete the comment entirely; locate the
comment adjacent to the safe_clamp test/assert to make the edit
(function/identifier: safe_clamp).
- Around line 5-6: The doc comment incorrectly states f32::clamp panics on NaN;
update the comment to say f32::clamp propagates NaN (returns NaN) rather than
panicking, and then briefly explain why safe_clamp exists: it provides
deterministic, real-time-safe behavior by handling NaN inputs explicitly (e.g.,
mapping NaN to a defined value or saturating to min/max) instead of letting a
NaN propagate. Edit the comment above the safe_clamp function to mention
f32::clamp's actual behavior and clearly state the deterministic handling policy
that safe_clamp implements.

In `@crates/openracing-watchdog/src/quarantine.rs`:
- Around line 168-172: The doc comment for is_empty() is misleading — it
currently says “Check if any plugins are quarantined” but the method (is_empty)
returns true when none are quarantined; update the doc string to match the
method contract (e.g., “Returns true if no plugins are quarantined” or “Check
whether the quarantine is empty”) so the documentation aligns with the behavior
of the quarantined field and the is_empty() method.

In `@crates/plugins/src/host.rs`:
- Around line 2-3: The file header currently embeds a machine-local URI
("file:///h:/Code/Rust/OpenRacing/docs/adr/0005-plugin-architecture.md");
replace that with a repo-stable reference (for example a relative path
"docs/adr/0005-plugin-architecture.md" or a canonical repo URL) so generated
docs don’t contain local file paths—update the string in the comment that
contains the ADR link to the chosen repo-stable URI.

In `@crates/plugins/src/lib.rs`:
- Line 3: The crate-level doc comment referencing the ADR uses a machine-local
file URL which breaks rustdoc links; update the doc line starting with "//!
Ref:" in crates::plugins::lib.rs to use a repository-relative path (e.g.,
"../docs/adr/0005-plugin-architecture.md" or
"docs/adr/0005-plugin-architecture.md") or a plain ADR identifier like
"ADR-0005: Plugin Architecture" so the link resolves for all users when
generating docs.

In `@crates/plugins/tests/plugin_rt_tests.rs`:
- Around line 20-36: The test test_plugin_dsp_output_nan_resilience duplicates
NaN sanitization logic instead of using the real implementation; update the test
to import and call openracing_hid_common::math::safe_clamp (or the actual
exported name) and apply it to PluginDspOutput.modified_ffb with bounds -1.0 and
1.0, then assert the returned sanitized value equals 0.0 (or the safe_clamp
expected result) so the test verifies production behavior rather than
re-implementing the logic.

In `@crates/service/src/device_service.rs`:
- Around line 42-43: The doc comment for the struct field `last_seen` overstates
its meaning; update the comment for `last_seen: Instant` to accurately state it
records the last time the device was observed or updated (e.g., during
enumeration, health checks, or other status updates) rather than strictly the
last successful communication; locate the `last_seen` field in the struct (the
field named last_seen in device_service.rs) and replace the comment with a
shorter, precise description such as "Timestamp of the last observed device
activity (enumeration, health check, or other updates)."

In `@crates/service/src/game_auto_configure.rs`:
- Around line 3-4: Doc comment at the top of the game_auto_configure module uses
a machine-local file:/// link; replace that with a repository-relative or plain
reference (e.g., "ADR-0008" or a relative path like
"../docs/adr/0008-game-auto-configure-telemetry-bridge.md") in the module-level
doc comment in crates/service/src/game_auto_configure.rs so rustdoc and CI see a
valid link; update the module doc (the //! comment block) accordingly.
- Around line 200-205: The loop in install_path_roots() that synchronously
probes all drive letters using Path::exists() risks long stalls (and is
currently called repeatedly from install path detection); replace this probing
with a single, cached enumeration of mounted drives on Windows by calling the
Win32 GetLogicalDrives/GetDriveType APIs (or the Rust crate wrapper) once and
deriving PathBuf roots from those bits, and ensure install_path_roots() is
updated to use that cached result so repeated calls (from the install path
detection loop) do not re-scan all letters; keep references to
install_path_roots() and any callers that expect Vec<PathBuf> and update the
logic to return only actually mounted drives instead of testing each letter with
Path::exists().

In `@crates/service/src/game_service.rs`:
- Around line 37-44: The new non-optional field telemetry_active on the public
struct GameStatusInfo will break deserialization for payloads that don't include
it; add #[serde(default)] to the telemetry_active field (and keep the existing
Deserialize derive on GameStatusInfo) so missing values default to false during
deserialization, ensuring backward compatibility for consumers that omit
telemetry_active.

In `@crates/service/src/game_telemetry_bridge.rs`:
- Around line 3-4: The module doc comment in game_telemetry_bridge.rs contains a
machine-local Windows file URI ("file:///h:/...") which is not portable; update
the doc comment (the top-line doc link referencing ADR-0008) to use a
repo-relative path (for example
"./docs/adr/0008-game-auto-configure-telemetry-bridge.md" or
"../docs/adr/0008-...") so rustdoc/CI and non-Windows environments can resolve
it, keeping the same markdown link text and format.

In `@crates/service/src/ipc_service.rs`:
- Around line 206-211: Replace the direct clamp casts for wheel_angle_mdeg and
wheel_speed_mrad_s with the NaN-safe helper: call
openracing_hid_common::math::safe_clamp on the scaled f32 values (e.g.
safe_clamp(t.wheel_angle_deg * 1000.0, i32::MIN as f32, i32::MAX as f32)) before
casting to i32 so wheel_angle_mdeg and wheel_speed_mrad_s handle NaN inputs
consistently with the rest of the HID path.

In `@crates/service/src/lib.rs`:
- Around line 2-4: Replace the machine-local file:/// links in the crate-level
docs that reference ADR-0002 and ADR-0008 with portable references — either use
repo-relative paths (e.g., ../../docs/adr/0002-ipc-transport.md) or simply the
ADR identifiers (ADR-0002, ADR-0008) so the references in the top-of-file doc
comment (the lines containing "Ref: [ADR-0002: IPC Transport]" and "Ref:
[ADR-0008: Game Auto-Configure]") resolve for other developers and in generated
docs.

In `@crates/service/src/profile_service.rs`:
- Around line 293-294: The field doc for cached_profiles is inaccurate: it says
"Number of profiles currently loaded in memory" but the value is actually
populated from the repository listing (profiles.len()); update the documentation
or the assignment to reflect the actual meaning. Locate the cached_profiles
field (pub cached_profiles: usize) in profile_service.rs and either change its
docstring to something like "Number of profiles reported by the repository
listing" or modify the code that sets cached_profiles (where profiles.len() is
used) to actually count in-memory entries if you intend the original meaning;
ensure any reference to profiles.len() or the method that populates
cached_profiles is adjusted to match the new doc meaning.

In `@crates/telemetry-adapters/tests/protocol_deep_tests.rs`:
- Around line 1039-1041: The current check uses
std::env::var_os("CI").is_some(), which treats any CI value (including "0" or
"false") as true; replace it with a truthy boolean check that reads the variable
string and interprets common falsey values. Update the test to use
std::env::var("CI").ok().map_or(false, |v| { let v =
v.trim().to_ascii_lowercase(); v == "1" || v == "true" || v == "yes" }) (or
equivalent) instead of var_os(...).is_some() so only explicit truthy values
enable the CI branch.

In `@crates/telemetry-config-writers/src/lib.rs`:
- Around line 14-26: The ForzaHorizon4ConfigWriter, ForzaHorizon5ConfigWriter,
and RFactor1ConfigWriter still construct "Documents/..." paths using
game_path.join(...) which bypasses the new Windows logic in resolve_game_path;
update each writer to call resolve_game_path(game_path, "<relative_path>") (or
pass the variable that holds the relative string) instead of
game_path.join(relative) so the "%USERPROFILE%\\Documents" handling on Windows
is applied consistently; ensure you pass the same relative_path strings those
writers currently use and remove any manual path separators so resolve_game_path
can normalize them.
- Around line 14-25: The resolve_game_path function currently uses
%USERPROFILE%/Documents on Windows which is incorrect for
redirected/OneDrive-managed profiles; replace the USERPROFILE-based branch
(inside the #[cfg(windows)] block) with a proper Known Folder lookup for
Documents (e.g. call SHGetKnownFolderPath / FOLDERID_Documents via the windows
or winapi crate) and use the returned path as the base, falling back to the
existing USERPROFILE approach only if the Known Folder lookup fails; keep the
function signature and behavior (returning a PathBuf and joining the remaining
stripped path), and ensure you handle UTF-16->OsString conversion and path
separator normalization when joining the stripped relative path in
resolve_game_path.

---

Outside diff comments:
In `@crates/engine/src/engine.rs`:
- Around line 1202-1224: Tests currently call .expect() when parsing DeviceId in
create_capturing_device and create_test_config; replace these panics by handling
the parse Result: either propagate the error by changing create_test_config to
return Result<EngineConfig, DeviceIdParseError> (or appropriate error type) and
map the parsed device_id into the config, and change create_capturing_device to
return Result<Box<dyn HidDevice>, _> (or accept a DeviceId argument) so callers
can handle failures; alternatively, provide a safe test-only constant DeviceId
created via DeviceId::from_str(...).ok_or(...)? and propagate errors instead of
using expect() in both create_capturing_device and create_test_config.
- Around line 1119-1128: The constructor CapturingDevice::new currently uses
TorqueNm::new(...).expect("valid torque"); change CapturingDevice::new to return
Result<Self, E> (choose the crate's existing error type or anyhow::Error) so you
can propagate the TorqueNm::new(...) failure with the ? operator instead of
calling expect; update the DeviceCapabilities::new call to use the result of
TorqueNm::new(...)?, adjust all callers of CapturingDevice::new to handle the
Result (propagate or map the error), and ensure any test helpers are updated
accordingly.

In `@crates/engine/src/hid/mod.rs`:
- Around line 272-288: The #[allow(clippy::expect_used)] attribute above the
to_device_capabilities function is stale and should be removed; edit the pub fn
to_device_capabilities(&self) -> DeviceCapabilities implementation to delete the
#[allow(clippy::expect_used)] line (and ensure no other expect() calls remain in
to_device_capabilities), leaving the function body unchanged (including the
unsafe TorqueNm::new_unchecked(clamped_nm) usage).

In `@crates/engine/src/pipeline.rs`:
- Around line 848-861: The tests module currently suppresses unwrap warnings and
defines a panic-based helper must(...) which contradicts our Result-returning
test pattern; remove #[allow(clippy::unwrap_used)] and replace panic-based must
with either using the ? operator directly in each test (change tests to return
Result<(), Box<dyn std::error::Error>>) or implement a small helper like
try_get<T, E>(r: Result<T,E>) -> Result<T,E> and propagate errors; update any
test functions referencing must to return Result and use ? (or try_get) so no
unwrap/expect/panic helpers remain—look for the tests module and the must
function to update.

In `@crates/engine/src/safety/integration.rs`:
- Around line 846-847: Remove the blanket #[allow(clippy::unwrap_used)] on mod
tests and refactor the tests inside the tests module to avoid unwrap()/expect():
convert failing tests to return Result<(), E> and use the ? operator for
fallible calls or replace unwrap()/expect() with explicit assertions (e.g.,
assert_eq!, assert!(matches!(...)), or map_err to propagate errors). Locate the
tests under mod tests in this file and for each test that calls
unwrap()/expect(), change its signature to return Result<(), Box<dyn
std::error::Error>> (or another concrete error type) and replace
unwrap()/expect() with ? or explicit assertion checks so the clippy allow
attribute can be removed. Ensure all tests compile and run as Result-returning
tests or use explicit assertions before deleting the
#[allow(clippy::unwrap_used)] attribute.

In `@crates/engine/src/two_phase_apply.rs`:
- Around line 495-525: The test test_two_phase_apply_multiple_pending currently
uses unwrap() on channel awaits (result_rx1.await.unwrap() and
result_rx2.await.unwrap()) which keeps the file depending on
#[allow(clippy::unwrap_used)]; change those awaits to propagate errors or assert
on their Result instead (e.g., let result1 = result_rx1.await? or
assert!(result_rx1.await.is_ok()) and then unwrap the success fields safely),
update similarly any other tests referenced (the ones around the other mentioned
ranges that call .await.unwrap()), and then remove the file-level
#[allow(clippy::unwrap_used)] so the module no longer uses unwrap/expect in
tests; reference the functions/values result_rx1, result_rx2, and
test_two_phase_apply_multiple_pending when making the edits.

In `@crates/hbp/src/lib.rs`:
- Around line 447-454: The tests (prop_two_byte_report_always_parses,
prop_four_byte_nonzero_id_uses_prefixed_layout,
prop_three_byte_zero_first_uses_raw_layout, prop_raw_encoding_round_trip) call
.expect("just checked is_some") on the result of
parse_hbp_usb_report_best_effort; replace those .expect() calls with pattern
matching (e.g., after prop_assert!(parsed.is_some()) use if let Some(sample) =
parsed { ... } else { prop_assert!(false, "parsed was None") }) so you
destructure the Some value without using unwrap/expect and then perform the
existing prop_assert_eq! checks on sample. Ensure each test uses the same if-let
pattern so the crate-level clippy lint for unwrap_used is satisfied.

In `@crates/hid-vrs-protocol/src/output.rs`:
- Around line 409-417: Change the test_torque_to_magnitude unit test to be a
Result-returning test to match project guidelines: update the function signature
to return Result<(), Box<dyn std::error::Error>> (or the same Result type used
by other tests in this file), keep the existing assert_eq! calls, and add a
final Ok(()) return; this uses the existing torque_to_magnitude function name to
locate the test and ensures the test follows the Result-returning pattern used
elsewhere.

In `@crates/ks/src/lib.rs`:
- Around line 67-90: KsAxisSource's parsing currently always uses parse_i16
causing unsigned bindings to be misinterpreted; update the consumer (the parse()
callsite that iterates encoder and rotary bindings) to check the
KsAxisSource.signed flag and call parse_u16 when signed is false (or return an
error/None for unsigned where appropriate), or alternatively reject unsigned
bindings before calling parse_i16. Locate usages of
KsAxisSource::parse_i16/parse_u16 (and the parse_bytes helper) and gate them on
self.signed so unsigned sources use parse_u16 (and are converted/handled
deliberately) while signed sources use parse_i16 to avoid wrapping high unsigned
values into negatives.

In `@crates/openracing-errors/tests/comprehensive_tests.rs`:
- Around line 536-555: The test's all_rt array is missing
RTError::AccessViolation so the From<RTError> for OpenRacingError conversion
isn't covered; update the test (rt_error_into_openracing_error) by adding
RTError::AccessViolation to the all_rt array alongside the other RTError
variants, then ensure the loop still converts each variant via let ore:
OpenRacingError = (*rt).into(); and asserts category() == ErrorCategory::RT and
that ore.to_string() contains rt.to_string().
- Around line 23-42: The test all_variants_have_nonempty_display is missing the
new RTError::AccessViolation variant, so update the variants array in that test
to include RTError::AccessViolation (alongside the existing
RTError::DeviceDisconnected, RTError::TorqueLimit, etc.) so the loop that calls
v.to_string() verifies the Display text for AccessViolation as well.

In `@crates/openracing-native-plugin/src/spsc.rs`:
- Around line 283-339: Replace panic-causing expect() calls in the tests by
converting each test function to return Result<(), Box<dyn std::error::Error>>
and using the ? operator for fallible calls; specifically update
test_spsc_channel_creation, test_spsc_write_read, test_spsc_ring_buffer, and
test_shutdown_flag to return Result and replace .expect("...") with ? on
SpscChannel::new, SpscChannel::with_capacity, writer.write, and reader.read,
keeping the same assertions (e.g., assert_eq!, assert!, etc.) and call Ok(()) at
the end of each test.

In `@crates/schemas/build.rs`:
- Around line 67-71: The create_protobuf_stubs() function is a no-op and its
call is dead code that misleads readers about the fallback; remove the
create_protobuf_stubs() function and any call sites (search for
create_protobuf_stubs()) so the code relies on the committed generated file
crates/schemas/src/generated/wheel.v1.rs as the real fallback, and update the
error message that currently says "Protobuf functionality will be disabled"
(search for that exact string) to clearly state that protoc failed but the build
will continue using the committed generated protobuf sources as a fallback.

In `@crates/schemas/src/ipc_conversion.rs`:
- Around line 564-577: The helpers must and must_some currently call panic! on
error which violates the no-panic test guideline; change them to return Results
(e.g., fn must<T, E: std::fmt::Debug>(r: Result<T,E>) -> Result<T, Box<dyn
std::error::Error>> and fn must_some<T>(o: Option<T>, msg: &str) -> Result<T,
Box<dyn std::error::Error>>) or to use assertions in tests that call them;
update all callers in tests to propagate errors by making tests return
Result<(), Box<dyn Error>> and using ? to propagate failures, or replace calls
with explicit assert!(r.is_ok()) / assert_eq!(...) so failures produce test
assertions rather than panics (refer to functions named must and must_some when
locating the helpers and their call sites).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 390cd34d-20b5-48c7-ace8-249617b061e7

📥 Commits

Reviewing files that changed from the base of the PR and between 76cc50b and 8692065.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (127)

• .gitignore
• check_errors.txt
• check_errors_v3.txt
• check_errors_v4.txt
• check_errors_v5.txt
• check_errors_v6.txt
• check_errors_v7.txt
• check_errors_v8.txt
• check_errors_v9.txt
• check_output.json
• check_output_v10.txt
• check_output_v11.txt
• check_output_v12.txt
• check_output_v13.txt
• check_output_v14.txt
• check_output_v15.txt
• check_output_v16.txt
• check_output_v17.txt
• check_output_v18.txt
• check_output_v19.txt
• check_output_v2.txt
• check_output_v20.txt
• check_output_v21.txt
• crates/changelog/src/lib.rs
• crates/compat/Cargo.toml
• crates/compat/src/telemetry_compat.rs
• crates/engine/Cargo.toml
• crates/engine/proptest-regressions/hid/windows_property_tests.txt
• crates/engine/src/compat_impl.rs
• crates/engine/src/device.rs
• crates/engine/src/engine.rs
• crates/engine/src/ffb.rs
• crates/engine/src/hid/mod.rs
• crates/engine/src/hid/rt_stream.rs
• crates/engine/src/hid/windows.rs
• crates/engine/src/hid/windows_property_tests.rs
• crates/engine/src/lib.rs
• crates/engine/src/pipeline.rs
• crates/engine/src/policies.rs
• crates/engine/src/ports.rs
• crates/engine/src/prelude.rs
• crates/engine/src/protocol.rs
• crates/engine/src/rt.rs
• crates/engine/src/safety.rs
• crates/engine/src/safety/fault_injection.rs
• crates/engine/src/safety/integration.rs
• crates/engine/src/scheduler.rs
• crates/engine/src/two_phase_apply.rs
• crates/hbp/src/lib.rs
• crates/hid-asetek-protocol/src/output.rs
• crates/hid-cammus-protocol/src/report.rs
• crates/hid-fanatec-protocol/Cargo.toml
• crates/hid-fanatec-protocol/src/output.rs
• crates/hid-fanatec-protocol/src/slots.rs
• crates/hid-fanatec-protocol/src/types.rs
• crates/hid-logitech-protocol/Cargo.toml
• crates/hid-logitech-protocol/src/output.rs
• crates/hid-logitech-protocol/src/slots.rs
• crates/hid-moza-protocol/Cargo.toml
• crates/hid-moza-protocol/src/direct.rs
• crates/hid-moza-protocol/src/lib.rs
• crates/hid-moza-protocol/src/protocol.rs
• crates/hid-moza-protocol/src/standalone.rs
• crates/hid-simagic-protocol/Cargo.toml
• crates/hid-simagic-protocol/src/lib.rs
• crates/hid-simagic-protocol/src/output.rs
• crates/hid-simucube-protocol/src/output.rs
• crates/hid-thrustmaster-protocol/Cargo.toml
• crates/hid-thrustmaster-protocol/src/output.rs
• crates/hid-vrs-protocol/Cargo.toml
• crates/hid-vrs-protocol/src/output.rs
• crates/hid-vrs-protocol/src/types.rs
• crates/integration-tests/src/acceptance.rs
• crates/integration-tests/src/common.rs
• crates/integration-tests/tests/mutation_coverage_tests.rs
• crates/ks/src/lib.rs
• crates/moza-wheelbase-report/src/lib.rs
• crates/openracing-capture-format/src/synthetic.rs
• crates/openracing-errors/src/rt.rs
• crates/openracing-errors/tests/comprehensive_tests.rs
• crates/openracing-errors/tests/error_handling_deep_tests.rs
• crates/openracing-errors/tests/exhaustiveness.rs
• crates/openracing-errors/tests/proptest_errors.rs
• crates/openracing-hid-common/Cargo.toml
• crates/openracing-hid-common/src/lib.rs
• crates/openracing-hid-common/src/math.rs
• crates/openracing-native-plugin/src/error.rs
• crates/openracing-native-plugin/src/plugin.rs
• crates/openracing-native-plugin/src/spsc.rs
• crates/openracing-scheduler/src/windows.rs
• crates/openracing-watchdog/src/quarantine.rs
• crates/openracing-watchdog/src/watchdog.rs
• crates/pidff-common/src/lib.rs
• crates/plugins/src/host.rs
• crates/plugins/src/lib.rs
• crates/plugins/src/wasm.rs
• crates/plugins/tests/plugin_rt_tests.rs
• crates/schemas/Cargo.toml
• crates/schemas/build.rs
• crates/schemas/src/domain.rs
• crates/schemas/src/ipc_conversion.rs
• crates/service/Cargo.toml
• crates/service/src/device_service.rs
• crates/service/src/game_auto_configure.rs
• crates/service/src/game_integration.rs
• crates/service/src/game_integration_minimal.rs
• crates/service/src/game_service.rs
• crates/service/src/game_telemetry_bridge.rs
• crates/service/src/ipc.rs
• crates/service/src/ipc_service.rs
• crates/service/src/lib.rs
• crates/service/src/profile_service.rs
• crates/service/src/safety_service.rs
• crates/simplemotion-v2/src/lib.rs
• crates/srp/src/lib.rs
• crates/telemetry-adapters/src/iracing.rs
• crates/telemetry-adapters/src/pcars2.rs
• crates/telemetry-adapters/tests/protocol_deep_tests.rs
• crates/telemetry-config-writers/src/lib.rs
• crates/telemetry-config/src/support.rs
• error_check.txt
• fatal.txt
• fatal_summary.txt
• final_test_results.txt
• workspace-hack/Cargo.toml
• workspace-hack/build.rs
• workspace-hack/src/lib.rs

💤 Files with no reviewable changes (1)

• crates/openracing-hid-common/Cargo.toml

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Build log documents multiple blocking issues for Windows platform.

This comprehensive build log shows the Windows HID implementation (crates/engine/src/hid/windows.rs) has numerous unresolved dependencies:

• Missing windows crate API features (WriteFile, Av* thread functions, Pnp module)
• Module organization issues (types expected in crate::input vs actual locations)
• Missing Seqlock type in hid module

These build failures must be resolved before the PR can be merged. Consider either:

1. Enabling required windows crate features in Cargo.toml
2. Adding #[cfg(target_os = "windows")] gates if Windows support is incomplete
3. Stubbing out the Windows module until dependencies are resolved

🧰 Tools

🪛 LanguageTool

[typographical] ~809-~809: To join two clauses or introduce examples, consider using an em dash.
Context: ...imilar name exists in the module | 48 - HidDevice, HidDeviceInfo, HidPort, M...

(DASH_RULE)

---

[typographical] ~814-~814: To join two clauses or introduce examples, consider using an em dash.
Context: ...imilar name exists in the module | 48 - HidDevice, HidDeviceInfo, HidPort, M...

(DASH_RULE)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@check_errors_v5.txt` around lines 1 - 903, The Windows HID file (windows.rs)
fails to compile due to missing windows crate APIs, misplaced imports, and a
missing Seqlock; fix by enabling the needed windows crate features in Cargo.toml
(add Win32::System::IO for WriteFile, Win32::System::WindowsProgramming for
AvSet/AvRevert*, Win32::UI::Pnp, etc.), or wrap the entire module with
#[cfg(target_os = "windows")] and provide a non-Windows stub; update imports in
crates::engine::src::hid::windows.rs to reference the correct public re-exports
(replace crate::input::{MozaInputState, TelemetryData} with the proper paths
such as crate::hid::MozaInputState or racing_wheel_schemas::... and fix vendor
imports like encode_torque_report_for_device/DeviceTelemetryReport to their
actual module or re-export), and either implement or import Seqlock (or remove
its usage) so Seqlock is available to the module; after enabling windows
features and correcting import paths (e.g., GetOverlappedResult, OVERLAPPED,
WriteFile, AvSetMmThreadCharacteristicsW, AvRevertMmThreadCharacteristics, Pnp
items, DeviceEvent/DeviceHealthStatus/HidDevice names) re-run cargo build to
ensure errors are resolved.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the windows crate features enabled for the engine crate
rg -n "windows\s*=" crates/engine/Cargo.toml -A 10

# Check the actual import in windows.rs
rg -n "Win32::UI::Pnp" crates/engine/src/hid/windows.rs -B 2 -A 2

# Check what Pnp-related features are available
rg -n "Pnp" crates/engine/Cargo.toml

Repository: EffortlessMetrics/OpenRacing

Length of output: 477

---

🏁 Script executed:

# Get the complete windows crate features from Cargo.toml
cat crates/engine/Cargo.toml | sed -n '/^windows = /,/^]/p'

Repository: EffortlessMetrics/OpenRacing

Length of output: 605

---

🏁 Script executed:

# Check the windows.rs file to see the actual import lines and context
head -50 crates/engine/src/hid/windows.rs | cat -n

Repository: EffortlessMetrics/OpenRacing

Length of output: 2529

---

🏁 Script executed:

# Verify if check_output_v10.txt file exists and if it should be in version control
git ls-files | grep -E "(check_output|\.txt$)" | head -20

Repository: EffortlessMetrics/OpenRacing

Length of output: 476

---

🏁 Script executed:

# Search for any Pnp references in windows.rs
rg -n "Pnp" crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 54

---

🏁 Script executed:

# Search for AvRevertMmThreadCharacteristics in windows.rs
rg -n "AvRevertMmThreadCharacteristics" crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 54

---

🏁 Script executed:

# Check if these are build log artifacts that shouldn't be committed
git log --oneline crates/engine/check_output_v10.txt 2>&1 | head -5

Repository: EffortlessMetrics/OpenRacing

Length of output: 286

---

🏁 Script executed:

# Check the actual location and git status of check_output_v10.txt
git status check_output_v10.txt 2>&1 || echo "---"
find . -name "check_output_v10.txt" -type f 2>/dev/null

Repository: EffortlessMetrics/OpenRacing

Length of output: 160

---

🏁 Script executed:

# Check if the file is actually in git ls-files (tracked)
git ls-files | grep "check_output_v10.txt"

Repository: EffortlessMetrics/OpenRacing

Length of output: 92

---

🏁 Script executed:

# Get more details about the file - is it untracked?
git status --porcelain | grep "check_output_v10.txt"

Repository: EffortlessMetrics/OpenRacing

Length of output: 54

---

Remove build log artifacts from version control.

Build output files like check_output_v10.txt should not be committed to the repository. These are CI/debug artifacts that clutter the codebase and become stale quickly. Additionally, multiple similar log files (check_errors_v3.txt through v9.txt, check_output_v2.txt through v19.txt, etc.) are tracked in git—all should be removed and added to .gitignore.

Note: The specific build errors shown in this log (unresolved windows::Win32::UI::Pnp and unused AvRevertMmThreadCharacteristics) do not appear in the current crates/engine/src/hid/windows.rs, suggesting these issues have already been resolved in the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@check_output_v10.txt` around lines 1 - 18, Remove the committed build log
artifacts (e.g., check_output_v10.txt and all similar files like
check_output_v2..v19.txt and check_errors_v3..v9.txt) from the repository and
add appropriate ignore patterns to .gitignore so they aren't re-added; remove
them from git history/staging with git rm (or git rm --cached) and commit the
deletions, then add patterns such as check_output*.txt and check_errors*.txt to
.gitignore to prevent future commits. Ensure you only delete/generated log files
and not source files—verify filenames before removing and commit the .gitignore
change together with the removals.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Build log files should not be committed to the repository.

This file appears to be a debugging artifact from development. Build output logs are typically ephemeral and should not be version-controlled. Consider:

1. Adding check_output*.txt to .gitignore
2. Removing these log files from the PR

If these logs are intentionally preserved for documentation purposes (e.g., tracking CI failures), they should be moved to a dedicated location with proper context explaining their purpose.

🧰 Tools

🪛 LanguageTool

[typographical] ~21-~21: To join two clauses or introduce examples, consider using an em dash.
Context: ...imilar name exists in the module | 37 - RegisterDeviceNotificationW, Unregis...

(DASH_RULE)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@check_output_v14.txt` around lines 1 - 35, Committed build log
check_output_v14.txt should be removed and ignored; delete the file from the PR
and add a gitignore entry. Remove the file from the repository (git rm
check_output_v14.txt or git rm --cached if you want to keep locally), add a
pattern like check_output*.txt to .gitignore, commit the .gitignore change and
the removal, and push; if these logs are intentionally kept, move
check_output_v14.txt into a documented location (docs/ci or diagnostics/) and
update README explaining purpose instead of keeping it at repo root.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check windows crate feature configuration

echo "=== Checking Cargo.toml for windows features ==="
rg -n "Win32_Devices_DeviceAndDriverInstallation" --type toml

echo ""
echo "=== Checking windows crate version and features in engine ==="
fd -t f "Cargo.toml" -x grep -l "windows" {} \; | xargs -I{} sh -c 'echo "=== {} ===" && cat {}'  | grep -A 20 '\[.*dependencies.*\]' | grep -A 10 "windows"

Repository: EffortlessMetrics/OpenRacing

Length of output: 10531

---

🌐 Web query:

Which Windows crate module contains RegisterDeviceNotificationW HDEVNOTIFY in Rust windows-rs?

💡 Result:

The Windows crate module containing RegisterDeviceNotificationW and HDEVNOTIFY is windows::Win32::UI::WindowsAndMessaging.

Citations:

• 1: https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/UI/WindowsAndMessaging/index.html
• 2: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-registerdevicenotificationa

---

🏁 Script executed:

cat crates/engine/Cargo.toml | sed -n '/\[target.*windows.*dependencies\]/,/^\[/p' | head -50

Repository: EffortlessMetrics/OpenRacing

Length of output: 722

---

Source code imports from wrong module; features are already enabled.

The Win32_Devices_DeviceAndDriverInstallation and Win32_UI_WindowsAndMessaging features are already correctly enabled in crates/engine/Cargo.toml. The build failure is not due to missing features, but because the code at crates/engine/src/hid/windows.rs:32 attempts to import RegisterDeviceNotificationW from Win32::Devices::DeviceAndDriverInstallation when it should be imported from Win32::UI::WindowsAndMessaging.

Also remove the unused import AvRevertMmThreadCharacteristics at line 23.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@check_output_v16.txt` around lines 1 - 26, The code is importing
RegisterDeviceNotificationW from the wrong module and keeping an unused
AvRevertMmThreadCharacteristics import; update the import block in windows.rs so
RegisterDeviceNotificationW is imported from
windows::Win32::UI::WindowsAndMessaging (not
windows::Win32::Devices::DeviceAndDriverInstallation) and remove
AvRevertMmThreadCharacteristics from the imported symbols (keep
AvSetMmThreadCharacteristicsW if still needed); ensure the import list
references the correct module and only the used symbols (e.g.,
RegisterDeviceNotificationW, AvSetMmThreadCharacteristicsW).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Build log files should not be committed; address the warnings in source files.

This build log should not be version-controlled. However, it reveals actionable warnings that should be addressed in the actual source files:

1. Dead code in crates/engine/src/hid/windows.rs: SendableHdevnotify and SendableHwnd have fields that are never read. Either use these fields or mark them with #[allow(dead_code)] if they're intentionally reserved for future use.

2. Unused function in crates/engine/src/ports.rs: DeviceInputs::from_moza_input_state is never used. Consider removing it or adding #[allow(dead_code)] with a comment explaining its intended future use.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@check_output_v2.txt` around lines 1 - 40, The committed build log contains
warnings about unused code; remove the log file from version control and fix the
source warnings: either use or mark as intentionally unused the tuple fields in
SendableHdevnotify and SendableHwnd (the structs named
SendableHdevnotify(HDEVNOTIFY) and SendableHwnd(HWND)) by adding
#[allow(dead_code)] with a comment if they’re reserved, and either remove
DeviceInputs::from_moza_input_state or annotate it with #[allow(dead_code)] and
a short comment explaining its future use so the compiler warnings are resolved.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use portable ADR references in the crate docs.

These file:///h:/Code/... links are machine-local and won't resolve for other developers or in generated docs. A repo-relative link or plain ADR identifier keeps the crate docs portable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/lib.rs` around lines 2 - 4, Replace the machine-local
file:/// links in the crate-level docs that reference ADR-0002 and ADR-0008 with
portable references — either use repo-relative paths (e.g.,
../../docs/adr/0002-ipc-transport.md) or simply the ADR identifiers (ADR-0002,
ADR-0008) so the references in the top-of-file doc comment (the lines containing
"Ref: [ADR-0002: IPC Transport]" and "Ref: [ADR-0008: Game Auto-Configure]")
resolve for other developers and in generated docs.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

cached_profiles doc does not match current computation

Line 293 describes an in-memory cache count, but current logic sets cached_profiles from repository listing size (profiles.len()). This can mislead API consumers.

🛠️ Suggested doc fix

-    /// Number of profiles currently loaded in memory
+    /// Number of profiles returned by the latest repository snapshot
     pub cached_profiles: usize,

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/// Number of profiles currently loaded in memory
	pub cached_profiles: usize,
	/// Number of profiles returned by the latest repository snapshot
	pub cached_profiles: usize,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/profile_service.rs` around lines 293 - 294, The field doc
for cached_profiles is inaccurate: it says "Number of profiles currently loaded
in memory" but the value is actually populated from the repository listing
(profiles.len()); update the documentation or the assignment to reflect the
actual meaning. Locate the cached_profiles field (pub cached_profiles: usize) in
profile_service.rs and either change its docstring to something like "Number of
profiles reported by the repository listing" or modify the code that sets
cached_profiles (where profiles.len() is used) to actually count in-memory
entries if you intend the original meaning; ensure any reference to
profiles.len() or the method that populates cached_profiles is adjusted to match
the new doc meaning.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Parse CI as a truthy boolean instead of presence-only.

Using std::env::var_os("CI").is_some() skips timing tests even when CI is set to "0"/"false", which can unintentionally disable these tests in non-CI environments.

💡 Proposed fix

-        if std::env::var_os("CI").is_some() {
+        if std::env::var("CI")
+            .map(|v| {
+                matches!(
+                    v.trim().to_ascii_lowercase().as_str(),
+                    "1" | "true" | "yes" | "on"
+                )
+            })
+            .unwrap_or(false)
+        {
             return true;
         }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if std::env::var_os("CI").is_some() {
	return true;
	}
	if std::env::var("CI")
	.map(|v| {
	matches!(
	v.trim().to_ascii_lowercase().as_str(),
	"1" | "true" | "yes" | "on"
	)
	})
	.unwrap_or(false)
	{
	return true;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-adapters/tests/protocol_deep_tests.rs` around lines 1039 -
1041, The current check uses std::env::var_os("CI").is_some(), which treats any
CI value (including "0" or "false") as true; replace it with a truthy boolean
check that reads the variable string and interprets common falsey values. Update
the test to use std::env::var("CI").ok().map_or(false, |v| { let v =
v.trim().to_ascii_lowercase(); v == "1" || v == "true" || v == "yes" }) (or
equivalent) instead of var_os(...).is_some() so only explicit truthy values
enable the CI branch.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Complete the Documents/ migration for all writers.

This helper fixes Windows path resolution only for call sites that use it, but ForzaHorizon4ConfigWriter, ForzaHorizon5ConfigWriter, and RFactor1ConfigWriter in this same module still write Documents/... paths via game_path.join(...). Those integrations will keep writing to the wrong location on Windows while the rest of the file now uses %USERPROFILE%\Documents.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 14 - 26, The
ForzaHorizon4ConfigWriter, ForzaHorizon5ConfigWriter, and RFactor1ConfigWriter
still construct "Documents/..." paths using game_path.join(...) which bypasses
the new Windows logic in resolve_game_path; update each writer to call
resolve_game_path(game_path, "<relative_path>") (or pass the variable that holds
the relative string) instead of game_path.join(relative) so the
"%USERPROFILE%\\Documents" handling on Windows is applied consistently; ensure
you pass the same relative_path strings those writers currently use and remove
any manual path separators so resolve_game_path can normalize them.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

USERPROFILE\\Documents is not the Windows Documents folder.

Hard-coding %USERPROFILE%\Documents breaks on redirected/OneDrive-managed profiles and other known-folder overrides, so configs can be written and validated against the wrong path even when the game is installed correctly. This needs a real Documents-folder lookup instead of an environment-variable guess. As per coding guidelines, "Avoid OS-specific assumptions unless the module is platform-specific; use platform-specific APIs (MMCSS on Windows, SCHED_FIFO on Linux, thread_policy_set on macOS) only in designated modules".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 14 - 25, The
resolve_game_path function currently uses %USERPROFILE%/Documents on Windows
which is incorrect for redirected/OneDrive-managed profiles; replace the
USERPROFILE-based branch (inside the #[cfg(windows)] block) with a proper Known
Folder lookup for Documents (e.g. call SHGetKnownFolderPath / FOLDERID_Documents
via the windows or winapi crate) and use the returned path as the base, falling
back to the existing USERPROFILE approach only if the Known Folder lookup fails;
keep the function signature and behavior (returning a PathBuf and joining the
remaining stripped path), and ensure you handle UTF-16->OsString conversion and
path separator normalization when joining the stripped relative path in
resolve_game_path.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/ui/src/commands.rs:357 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:189 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:192 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:211 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:608 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:217 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

crates/integration-tests/src/common.rs (2)

56-65: ⚠️ Potential issue | 🟡 Minor

Align history capacity with the enforced limit to avoid realloc churn.

Line 56 initializes with capacity 1000, but Line 64 enforces a retained size near 10000. This repeatedly grows the buffer under sustained updates.

Suggested patch

-            torque_history: VecDeque::with_capacity(1000),
+            torque_history: VecDeque::with_capacity(10000),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/integration-tests/src/common.rs` around lines 56 - 65, The VecDeque
initial capacity is 1000 but set_torque enforces a retained size up to 10000,
causing repeated growth; update the initializer for torque_history
(VecDeque::with_capacity) to use 10000 (matching the pruning threshold in
set_torque) so capacity and the enforced limit align and avoid realloc churn.

---

69-82: 🧹 Nitpick | 🔵 Trivial

Remove temporary allocation in verify_torque_ramp_to_zero.

The collect() on Line 70-74 allocates a Vec only to read the last element. You can scan directly from the back.

Suggested patch

-        let relevant_history: Vec<_> = self
-            .torque_history
-            .iter()
-            .filter(|(t, _)| *t >= start_time)
-            .collect();
-
-        relevant_history
-            .last()
+        self.torque_history
+            .iter()
+            .rev()
+            .find(|(t, _)| *t >= start_time)
             .map(|(end_time, last_torque)| {
                 last_torque.abs() < 1e-6 && (*end_time - start_time) <= duration
             })
             .unwrap_or(false)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/integration-tests/src/common.rs` around lines 69 - 82, The function
verify_torque_ramp_to_zero currently collects a temporary Vec from
torque_history just to read the last element; replace that allocation by
scanning from the back instead (e.g., use
self.torque_history.iter().rev().find(|(t, _)| *t >= start_time)) to get the
most recent (end_time, last_torque) without allocating, then apply the same
checks (last_torque.abs() < 1e-6 && (*end_time - start_time) <= duration) and
return false if none found; update the function verify_torque_ramp_to_zero to
use this reverse-find approach.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/engine/src/hid/mod.rs`:
- Around line 3-6: The module doc comment in hid::mod.rs currently points to a
workstation-local file URL; replace the hard-coded file:/// path with a
repository-hosted ADR link (for example a relative path or a canonical repo URL)
so rustdoc/CI can resolve it (update the reference line `[ADR-0007]:
file:///...` to something like `[ADR-0007]:
../docs/adr/0007-multi-vendor-hid-protocol-architecture.md` or the project's
GitHub/raw URL).

In `@crates/integration-tests/src/common.rs`:
- Around line 69-82: The test harness never starts the WheelService control
loop, so injected faults never trigger torque commands; update
TestHarness.start_service() to spawn WheelService.run() as a background task (or
ensure the service's event loop is started) after creating the WheelService so
that fault_flags set by inject_fault() are observed and processed;
alternatively, if you prefer not to run the loop in tests, call
WheelService::set_torque(0) (or the harness wrapper) immediately after
inject_fault() before calling verify_torque_ramp_to_zero() to simulate the
expected response; ensure whichever approach you pick integrates with the
existing TestHarness lifecycle and uses the same WheelService instance
referenced by verify_torque_ramp_to_zero().

In `@crates/openracing-native-plugin/tests/protocol_tests.rs`:
- Around line 372-380: Replace the loose substring check on err_msg with a
direct assertion of the concrete error variant: extract the error from result
(e.g., via result.err().unwrap() or result.unwrap_err()) and assert it matches
NativePluginError::FrameSizeMismatch (use pattern matching or downcast if the
error is boxed/trait object). Update the assertion around the result variable in
the test in protocol_tests.rs so the test fails unless the exact
NativePluginError::FrameSizeMismatch is produced.

---

Duplicate comments:
In `@crates/integration-tests/src/common.rs`:
- Around line 56-65: The VecDeque initial capacity is 1000 but set_torque
enforces a retained size up to 10000, causing repeated growth; update the
initializer for torque_history (VecDeque::with_capacity) to use 10000 (matching
the pruning threshold in set_torque) so capacity and the enforced limit align
and avoid realloc churn.
- Around line 69-82: The function verify_torque_ramp_to_zero currently collects
a temporary Vec from torque_history just to read the last element; replace that
allocation by scanning from the back instead (e.g., use
self.torque_history.iter().rev().find(|(t, _)| *t >= start_time)) to get the
most recent (end_time, last_torque) without allocating, then apply the same
checks (last_torque.abs() < 1e-6 && (*end_time - start_time) <= duration) and
return false if none found; update the function verify_torque_ramp_to_zero to
use this reverse-find approach.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e75298b6-b044-4ca8-860d-c3fa6675b52f

📥 Commits

Reviewing files that changed from the base of the PR and between 8692065 and 4a266ab.

📒 Files selected for processing (4)

• crates/engine/src/hid/mod.rs
• crates/integration-tests/src/common.rs
• crates/integration-tests/tests/integration_tests.rs
• crates/openracing-native-plugin/tests/protocol_tests.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use a portable ADR reference.

Line 6 hard-codes a workstation-local file:// path. That link will be dead in rustdoc/CI for everyone except the author machine and exposes a local filesystem layout; point it at a repo-hosted ADR reference instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/hid/mod.rs` around lines 3 - 6, The module doc comment in
hid::mod.rs currently points to a workstation-local file URL; replace the
hard-coded file:/// path with a repository-hosted ADR link (for example a
relative path or a canonical repo URL) so rustdoc/CI can resolve it (update the
reference line `[ADR-0007]: file:///...` to something like `[ADR-0007]:
../docs/adr/0007-multi-vendor-hid-protocol-architecture.md` or the project's
GitHub/raw URL).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== set_torque definitions/call sites ==="
rg -nP --type rust -C3 '\bset_torque\s*\('

echo
echo "=== fault injection call sites ==="
rg -nP --type rust -C3 '\binject_fault\s*\('

echo
echo "=== verify_torque_ramp_to_zero usage ==="
rg -nP --type rust -C3 '\bverify_torque_ramp_to_zero\s*\('

echo
echo "=== any direct torque-to-zero writes on fault paths ==="
rg -nP --type rust -C3 'fault|fault_flags|set_torque\(\s*0(\.0)?\s*\)'

Repository: EffortlessMetrics/OpenRacing

Length of output: 50386

---

🏁 Script executed:

sed -n '549,597p' crates/integration-tests/src/acceptance.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1615

---

🏁 Script executed:

# Find SafetyService fault handling
rg -nP --type rust 'report_fault|handle.*fault|fault.*response' crates/integration-tests/src/ -A 5

echo "=== TestHarness start_service ==="
rg -nP --type rust 'fn start_service' crates/integration-tests/src/ -A 10

echo "=== Check if SafetyService exists and auto-responds ==="
find . -name "*.rs" -path "*/safety/*" -o -name "*safety*.rs" | head -10

Repository: EffortlessMetrics/OpenRacing

Length of output: 7002

---

🏁 Script executed:

# Find WheelService and its control/tick loop
rg -nP --type rust 'impl.*WheelService|fn.*tick|fn.*process' crates/ --max-count=20 | grep -i 'wheel\|service'

echo "=== Look for control loop in WheelService ==="
find crates -name "*.rs" -exec grep -l "WheelService" {} \; | head -5

echo "=== Search for fault-to-torque-zero logic ==="
rg -nP --type rust 'fault.*set_torque|set_torque.*0|faults.*ramp' crates/integration-tests/src/ -A 3

Repository: EffortlessMetrics/OpenRacing

Length of output: 4162

---

🏁 Script executed:

# Find WheelService tick/update/control loop
rg -nP --type rust 'pub.*fn.*tick|async.*fn.*tick|fn.*update|fn.*control' crates/service/src/service.rs -A 8

echo "=== Look for WheelService structure and main methods ==="
rg -nP --type rust 'impl.*WheelService|pub fn new|pub async fn' crates/service/src/service.rs | head -20

Repository: EffortlessMetrics/OpenRacing

Length of output: 345

---

🏁 Script executed:

# Examine the run() method which is the control/event loop
sed -n '107,200p' crates/service/src/service.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 3370

---

🏁 Script executed:

# Check what start_service does - does it run the control loop?
sed -n '134,160p' crates/integration-tests/src/common.rs

echo "=== Check TestHarness structure for control loop/background tasks ==="
rg -nP --type rust 'struct TestHarness|impl TestHarness' crates/integration-tests/src/common.rs -A 15

echo "=== Look for automated torque zeroing on fault in device service ==="
rg -nP --type rust 'fault.*set_torque|torque.*fault|safe_torque' crates/service/src/ -B 2 -A 2

Repository: EffortlessMetrics/OpenRacing

Length of output: 15640

---

Confirmed: missing fault-response mechanism. The test does not start the service control loop.

TestHarness.start_service() creates the WheelService but never calls .run(), which means the device service and safety service event loops never start. Consequently, fault flag changes are not monitored or processed. The test injects a fault via inject_fault() (which only sets fault_flags), but without the background control loop, no torque command is issued. When verify_torque_ramp_to_zero() checks for post-fault torque samples, the history is empty, resulting in a false negative.

To fix: either (1) spawn the service's main loop in a background task during test setup, or (2) manually call set_torque(0) after fault injection to simulate the expected response, or (3) verify that the service's event loop is properly integrated into the harness lifecycle.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/integration-tests/src/common.rs` around lines 69 - 82, The test
harness never starts the WheelService control loop, so injected faults never
trigger torque commands; update TestHarness.start_service() to spawn
WheelService.run() as a background task (or ensure the service's event loop is
started) after creating the WheelService so that fault_flags set by
inject_fault() are observed and processed; alternatively, if you prefer not to
run the loop in tests, call WheelService::set_torque(0) (or the harness wrapper)
immediately after inject_fault() before calling verify_torque_ramp_to_zero() to
simulate the expected response; ensure whichever approach you pick integrates
with the existing TestHarness lifecycle and uses the same WheelService instance
referenced by verify_torque_ramp_to_zero().

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Assert the concrete error variant instead of matching message substrings.

This check is too permissive; unrelated errors containing “size”/“mismatch” would still pass. Assert NativePluginError::FrameSizeMismatch directly to keep the test strict.

Suggested fix

-    let err_msg = result
-        .as_ref()
-        .err()
-        .map(|e| e.to_string())
-        .unwrap_or_default();
-    assert!(
-        err_msg.contains("mismatch") || err_msg.contains("size"),
-        "Expected error about frame size mismatch, got: {err_msg}"
-    );
+    match result {
+        Err(NativePluginError::FrameSizeMismatch { expected, actual }) => {
+            assert_eq!(expected, 64);
+            assert_eq!(actual, 32);
+        }
+        Err(other) => panic!("Expected FrameSizeMismatch, got: {other}"),
+        Ok(_) => panic!("Expected frame size mismatch error"),
+    }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let err_msg = result
	.as_ref()
	.err()
	.map(|e| e.to_string())
	.unwrap_or_default();
	assert!(
	err_msg.contains("mismatch") || err_msg.contains("size"),
	"Expected error about frame size mismatch, got: {err_msg}"
	);
	match result {
	Err(NativePluginError::FrameSizeMismatch { expected, actual }) => {
	assert_eq!(expected, 64);
	assert_eq!(actual, 32);
	}
	Err(other) => panic!("Expected FrameSizeMismatch, got: {other}"),
	Ok(_) => panic!("Expected frame size mismatch error"),
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/openracing-native-plugin/tests/protocol_tests.rs` around lines 372 -
380, Replace the loose substring check on err_msg with a direct assertion of the
concrete error variant: extract the error from result (e.g., via
result.err().unwrap() or result.unwrap_err()) and assert it matches
NativePluginError::FrameSizeMismatch (use pattern matching or downcast if the
error is boxed/trait object). Update the assertion around the result variable in
the test in protocol_tests.rs so the test fails unless the exact
NativePluginError::FrameSizeMismatch is produced.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:189 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:192 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:211 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:608 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:217 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/ui/src/commands.rs:357 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[github-actions]

Code Coverage Summary

  651|     61|        for game in matrix.games.values() {
                                  ^1           ^1
  652|     62|            for ver in &game.versions {
                                     ^61
  653|     62|                if !ver.executable_patterns.is_empty() || !ver.config_paths.is_empty() {
                                                                        ^4
  654|     59|                    covered += 1;
  655|     59|                }
                              ^3
  656|       |            }
  657|       |        }
  658|       |        // Most versions should have at least one identification path
  659|      1|        assert!(
  660|      1|            covered >= 10,
  661|       |            "expected >= 10 versions with patterns, got {}",
  662|       |            covered
  663|       |        );
  664|      1|        Ok(())
  665|      1|    }
  666|       |}```

<!-- Sticky Pull Request Commentcoverage -->

[codecov]

Codecov Report

❌ Patch coverage is 72.97980% with 214 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/engine/src/policies.rs	51.25%	2 Missing and 37 partials ⚠️
crates/engine/src/two_phase_apply.rs	57.57%	0 Missing and 28 partials ⚠️
crates/engine/src/safety/fault_injection.rs	55.17%	0 Missing and 26 partials ⚠️
crates/schemas/src/domain.rs	60.60%	0 Missing and 26 partials ⚠️
crates/engine/src/device.rs	69.23%	0 Missing and 16 partials ⚠️
crates/service/src/service.rs	66.66%	10 Missing and 4 partials ⚠️
crates/engine/src/ffb.rs	64.51%	0 Missing and 11 partials ⚠️
crates/engine/src/safety/integration.rs	28.57%	7 Missing and 3 partials ⚠️
crates/engine/src/engine.rs	66.66%	0 Missing and 9 partials ⚠️
crates/service/src/ipc_service.rs	0.00%	6 Missing ⚠️
... and 10 more

📢 Thoughts on this report? Let us know!

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:189 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:192 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:211 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:608 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:217 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:744 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[coderabbitai]

Actionable comments posted: 7

♻️ Duplicate comments (3)

crates/plugins/tests/plugin_rt_tests.rs (1)

25-37: 🧹 Nitpick | 🔵 Trivial

Use shared safe_clamp instead of duplicating sanitization logic in test.

This test re-implements sanitization at Lines 33-37, so it can diverge from production behavior. Import and call the shared sanitizer directly.

Proposed fix

+use openracing_hid_common::math::safe_clamp;
 use racing_wheel_plugins::native::{PluginFrame, SharedMemoryHeader};
 use racing_wheel_plugins::{PluginDspOutput, PluginOutput};
 use std::mem::size_of;
@@
-    // Simulate engine sanitization
-    let sanitized_ffb = if output.modified_ffb.is_finite() {
-        output.modified_ffb.clamp(-1.0, 1.0)
-    } else {
-        0.0
-    };
+    let sanitized_ffb = safe_clamp(output.modified_ffb, -1.0, 1.0);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/plugins/tests/plugin_rt_tests.rs` around lines 25 - 37, Test
duplicates sanitization logic for PluginDspOutput.modified_ffb; replace the
ad-hoc if/is_finite/clamp block with a call to the shared sanitizer (safe_clamp)
used by production code: import or bring safe_clamp into scope in the test and
call safe_clamp(output.modified_ffb, -1.0, 1.0) (or the correct parameter order
used by the shared function) to compute sanitized_ffb so the test follows the
same behavior as the engine.

crates/telemetry-config-writers/src/lib.rs (2)

14-15: ⚠️ Potential issue | 🟠 Major

The Documents/... rollout is still partial.

ForzaHorizon4ConfigWriter, ForzaHorizon5ConfigWriter, and RFactor1ConfigWriter still build Documents/... targets with game_path.join(...), so they will bypass this helper entirely even after the resolver logic is corrected.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 14 - 15, The helper
resolve_game_path handles Windows "Documents/..." specially but several writers
still bypass it; update ForzaHorizon4ConfigWriter, ForzaHorizon5ConfigWriter,
and RFactor1ConfigWriter to call resolve_game_path(game_path, relative_path) (or
refactor their path construction to use the helper) instead of using
game_path.join(...), ensuring any "Documents/..." relative paths are routed
through the resolver so the special-casing is applied consistently.

---

22-29: ⚠️ Potential issue | 🟠 Major

Don't derive Documents from USERPROFILE.

%USERPROFILE%\Documents breaks on redirected or OneDrive-managed profiles, so this branch can still read and write the wrong location. Use the Windows Known Folder lookup (FOLDERID_Documents / SHGetKnownFolderPath, or an equivalent crate wrapper) and fall back only if that lookup fails. As per coding guidelines, "Avoid OS-specific assumptions unless the module is platform-specific; use platform-specific APIs (MMCSS on Windows, SCHED_FIFO on Linux, thread_policy_set on macOS) only in designated modules".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 22 - 29, The current
Windows branch that handles relative_path.strip_prefix("Documents/") incorrectly
builds the Documents path from the USERPROFILE env var; replace that logic to
call the Windows Known Folder API (FOLDERID_Documents via SHGetKnownFolderPath
or a maintained crate wrapper) to resolve the real Documents folder and join the
stripped path to it, and only fall back to constructing from USERPROFILE if the
Known Folder lookup fails; update the code around the
relative_path.strip_prefix("Documents/") branch in lib.rs to perform the Known
Folder lookup and use its result (or the fallback) when returning the final
PathBuf.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/engine/src/hid/windows.rs`:
- Around line 353-356: In cancel_and_drain(), replace the indefinite blocking
GetOverlappedResult(handle, &self.overlapped, &mut bytes_transferred, true) with
a finite wait using the OVERLAPPED event: use
WaitForSingleObject(self.overlapped.hEvent, TIMEOUT_MS) (or equivalent deadline
logic) and if it returns signaled call GetOverlappedResult(handle,
&self.overlapped, &mut bytes_transferred, false); if the wait times out, surface
a hard failure (log critical/panic) indicating CancelIoEx did not complete
within the deadline so the destructor can fail fast. Keep references to
CancelIoEx, self.overlapped, handle and bytes_transferred when implementing the
change.
- Around line 339-342: The code incorrectly treats handle.is_invalid() as proof
there is no pending overlapped write and clears self.write_pending and returns,
which can free OVERLAPPED and write_buffer while the OS may still deliver
completion; instead, when handle.is_invalid() and self.write_pending is true,
wait for the overlapped event (via WaitForSingleObject on the OVERLAPPED event)
or ensure pending I/O is canceled (CancelIoEx) and completion drained before
freeing buffers, rather than returning early; also replace the unbounded
GetOverlappedResult(..., true) wait with a bounded wait (use WaitForSingleObject
with a sensible timeout and then GetOverlappedResult) so the thread cannot block
forever—inspect and update the logic around the handle.is_invalid() branch, the
GetOverlappedResult call, and the places that clear write_pending or free the
write_buffer/OVERLAPPED to ensure buffers remain alive until completion is
processed.

In `@crates/plugins/tests/plugin_rt_tests.rs`:
- Around line 10-11: Update the inaccurate inline comment next to the
size_of::<PluginFrame>() assertion: either remove the leftover note and the
incorrect layout comment, or replace it with the correct field layout
(PluginFrame contains f32*3 + u64 + u32*2 + padding) so the comment matches the
actual struct; the change should be made adjacent to the
size_of::<PluginFrame>() assertion in the test.

In `@crates/service/src/service_tests.rs`:
- Around line 103-108: The test explicitly sets flags.enable_virtual_devices =
true even though FeatureFlags::default() already enables this; either remove the
redundant assignment or keep it but clarify intent—update the test around
FeatureFlags and WheelService::new_with_flags to either delete the explicit
flags.enable_virtual_devices = true line, or replace it with a short comment
stating that the test requires virtual devices and that FeatureFlags::default()
already provides that, so the assignment is for documentation only.

In `@crates/service/src/service.rs`:
- Around line 58-82: The code logs a warning twice when falling back to virtual
HID devices: once in the Err branch of
racing_wheel_engine::hid::create_hid_port() and again using real_port_failed
after creating the virtual port; remove redundancy by either deleting the second
warn! that uses real_port_failed or change its level/message to clearly
differentiate (e.g., error or a distinct “SERVICE DEGRADED” metric) so you only
emit one meaningful operational log for the fallback in the Hid initialization
flow that involves flags.enable_virtual_devices, create_hid_port(),
create_virtual_port(), and the real_port_failed flag.

In `@crates/service/src/system_config.rs`:
- Around line 220-232: FeatureFlags::default() sets enable_virtual_devices =
true but DevelopmentConfig::default() still uses false, causing inconsistent
behavior depending on whether callers use FeatureFlags::default() or
SystemConfig::default(); update the persisted/default config to match the
runtime default (or vice versa) so both constructors are identical —
specifically, change DevelopmentConfig::default() (the struct that holds
enable_virtual_devices in persisted settings) to set enable_virtual_devices =
true (or alter FeatureFlags::default() if the intended baseline is false) and
ensure any comments/tests referencing the previous value are updated to reflect
the chosen consistent default.

In `@crates/telemetry-config-writers/src/lib.rs`:
- Around line 16-19: The early return in resolve_game_path (and usages in
write_config) makes the Windows "Documents" remap unreachable because real
callers pass the install directory as game_path; instead, change the API to
separate install_root from a test-only documents_override (e.g., add an
Option<PathBuf> parameter or a distinct documents_root) and update
resolve_game_path to: always treat game_path as the install root, and only use
the documents_override when relative_path targets "Documents/..." (apply the
Windows remap logic when documents_override.is_some()), then update write_config
and all resolve_game_path call sites to provide the actual install directory as
game_path and the optional test Documents override where appropriate so the
remap works in tests without breaking real usage.

---

Duplicate comments:
In `@crates/plugins/tests/plugin_rt_tests.rs`:
- Around line 25-37: Test duplicates sanitization logic for
PluginDspOutput.modified_ffb; replace the ad-hoc if/is_finite/clamp block with a
call to the shared sanitizer (safe_clamp) used by production code: import or
bring safe_clamp into scope in the test and call safe_clamp(output.modified_ffb,
-1.0, 1.0) (or the correct parameter order used by the shared function) to
compute sanitized_ffb so the test follows the same behavior as the engine.

In `@crates/telemetry-config-writers/src/lib.rs`:
- Around line 14-15: The helper resolve_game_path handles Windows
"Documents/..." specially but several writers still bypass it; update
ForzaHorizon4ConfigWriter, ForzaHorizon5ConfigWriter, and RFactor1ConfigWriter
to call resolve_game_path(game_path, relative_path) (or refactor their path
construction to use the helper) instead of using game_path.join(...), ensuring
any "Documents/..." relative paths are routed through the resolver so the
special-casing is applied consistently.
- Around line 22-29: The current Windows branch that handles
relative_path.strip_prefix("Documents/") incorrectly builds the Documents path
from the USERPROFILE env var; replace that logic to call the Windows Known
Folder API (FOLDERID_Documents via SHGetKnownFolderPath or a maintained crate
wrapper) to resolve the real Documents folder and join the stripped path to it,
and only fall back to constructing from USERPROFILE if the Known Folder lookup
fails; update the code around the relative_path.strip_prefix("Documents/")
branch in lib.rs to perform the Known Folder lookup and use its result (or the
fallback) when returning the final PathBuf.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 32536aeb-a60e-4de4-a9f1-0ad1c6283243

📥 Commits

Reviewing files that changed from the base of the PR and between 4a266ab and 80f1c61.

📒 Files selected for processing (17)

• .github/workflows/integration-tests.yml
• crates/engine/src/hid/windows.rs
• crates/engine/src/hid/windows_property_tests.rs
• crates/plugins/tests/plugin_rt_tests.rs
• crates/service/src/daemon.rs
• crates/service/src/integration_tests.rs
• crates/service/src/service.rs
• crates/service/src/service_tests.rs
• crates/service/src/system_config.rs
• crates/service/tests/lifecycle_tests.rs
• crates/service/tests/service_hardening_tests.rs
• crates/service/tests/service_integration_tests.rs
• crates/service/tests/service_ipc_deep_tests.rs
• crates/service/tests/service_lifecycle_comprehensive_tests.rs
• crates/service/tests/service_lifecycle_deep_tests.rs
• crates/service/tests/service_lifecycle_hardening_tests.rs
• crates/telemetry-config-writers/src/lib.rs

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, check the file structure and examine the specific lines mentioned
wc -l crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 109

---

🏁 Script executed:

# Read the code sections mentioned: 329-360, 370-379, 339-342, 2946-2959
sed -n '329,360p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1393

---

🏁 Script executed:

# Check the Drop implementation for OverlappedWriteState
sed -n '370,385p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1054

---

🏁 Script executed:

# Check the WindowsHidDevice Drop implementation
sed -n '2946,2965p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 956

---

🏁 Script executed:

# Look for the OverlappedWriteState struct definition to understand its fields
rg -n "struct OverlappedWriteState" crates/engine/src/hid/windows.rs -A 10

Repository: EffortlessMetrics/OpenRacing

Length of output: 664

---

🏁 Script executed:

# Search for write_buffer and overlapped field declarations
rg -n "write_buffer|overlapped" crates/engine/src/hid/windows.rs | head -20

Repository: EffortlessMetrics/OpenRacing

Length of output: 1445

---

🌐 Web query:

Windows OVERLAPPED structure lifetime requirements CancelIoEx GetOverlappedResult

💡 Result:

The Windows OVERLAPPED structure must remain valid until the associated asynchronous I/O operation completes, regardless of whether it succeeds, fails, or is canceled. Do not free or reuse the structure before completion. Key requirements: - Allocate a unique OVERLAPPED structure per I/O operation (e.g., ReadFile, WriteFile). Microsoft docs explicitly warn: "A common mistake is to reuse an OVERLAPPED structure before the previous asynchronous operation has been completed. You should use a separate structure for each request." - After issuing an operation, the system takes ownership of the structure and may access it asynchronously (e.g., to update Internal/InternalHigh fields or signal hEvent). - Completion is signaled via: - hEvent (if set to a manual-reset event). - GetOverlappedResult(hFile, lpOverlapped, ..., TRUE) waits and checks Internal != STATUS_PENDING. - GetQueuedCompletionStatus for IOCP, returning the lpOverlapped pointer. - Completion routine (APC) if using ReadFileEx/etc. For CancelIoEx(hFile, lpOverlapped): - Marks the specific operation(s) matching lpOverlapped for cancellation (or all if NULL). - Does NOT wait for completion; returns immediately if cancel request queued successfully. - Critical: "The application must not free or reuse the OVERLAPPED structure associated with the canceled I/O operations until they have completed." - Canceled operations still complete (with ERROR_OPERATION_ABORTED), generating a completion notification. - Free/reuse only in the completion handler (e.g., after GetOverlappedResult succeeds or in IOCP callback). For GetOverlappedResult(hFile, lpOverlapped, ...): - Requires the lpOverlapped pointer to remain valid. - If bWait=TRUE, blocks until complete (Internal updated). - Validates the specific operation via pointer matching. - lpOverlapped must point to the original valid structure. Best practice (Raymond Chen/Microsoft DevBlogs): After CancelIoEx, always wait (e.g., WaitForSingleObject(hEvent, INFINITE) or GetOverlappedResult(..., TRUE)) before freeing. Heap-allocate OVERLAPPED with app data; free only in completion path. Violations cause races: system writes to freed memory, crashes, or corruption. Track pending OVERLAPPEDs (e.g., list/map) for shutdown: cancel all, drain completions, then free. No changes in Windows 10/11/Server 2022 docs (as of 2026-03-28).

Citations:

• 1: https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-overlapped
• 2: https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-cancelioex
• 3: https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-getoverlappedresult
• 4: https://devblogs.microsoft.com/oldnewthing/?p=11613
• 5: https://learn.microsoft.com/en-us/windows/win32/sync/synchronization-and-overlapped-input-and-output

---

🌐 Web query:

Win32 handle invalid pending I/O operation cleanup behavior

💡 Result:

When a Win32 handle becomes invalid (e.g., via CloseHandle), pending overlapped (asynchronous) I/O operations on that handle are canceled by the system. Closing the handle (specifically, the last handle to the underlying file object) triggers an IRP_MJ_CLEANUP in the kernel driver stack. Built-in Windows drivers complete pending IRPs with an error like STATUS_CANCELLED during cleanup, ensuring operations do not continue indefinitely. The I/O completions are delivered (e.g., to IOCP or OVERLAPPED event/APC) with ERROR_OPERATION_ABORTED, even after the handle is invalid. The OVERLAPPED structure must remain valid until completion notification is received and processed. CloseHandle itself does not block waiting for pending I/O; it returns promptly after invalidating the handle. Using an invalid handle in new API calls fails with ERROR_INVALID_HANDLE. This behavior is safe and standard for cleanup, starting reliably from Windows Vista (earlier versions depend on driver implementation).

Citations:

• 1: https://learn.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-closehandle
• 2: https://stackoverflow.com/questions/53490476/windows-safe-to-call-closehandle-when-an-overlapped-i-o-is-in-progress
• 3: https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/irp-mj-cleanup?redirectedfrom=MSDN
• 4: https://learn.microsoft.com/en-us/windows/win32/fileio/canceling-pending-i-o-operations
• 5: https://stackoverflow.com/questions/14766465/what-if-i-call-closehandleh-before-an-asynchronous-read-on-handle-h-has-not-fi
• 6: https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/dispatchcleanup-routines

---

Don't let release builds free the overlapped buffers without confirmed completion.

Lines 339–342 incorrectly assume the OS has cleaned up when the handle is invalid, but per Windows documentation, pending overlapped I/O still completes asynchronously after CloseHandle, delivering completion to the event handle with ERROR_OPERATION_ABORTED. The OVERLAPPED structure and write_buffer must remain valid until that completion is processed. Clearing write_pending and returning early without draining allows both to be freed while the kernel may still be signaling completion. Lines 370–379 only panic in debug builds, so release builds will silently drop the memory, creating a use-after-free window.

When a handle is invalid but I/O is pending, the code must wait on the event handle directly (e.g., WaitForSingleObject) rather than attempting GetOverlappedResult with an invalid handle. Alternatively, make pending+invalid-handle impossible by canceling before closing the handle.

Also address the unbounded blocking at line 351: GetOverlappedResult(..., true) has no timeout, though Win32 guarantees completion will arrive.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/hid/windows.rs` around lines 339 - 342, The code
incorrectly treats handle.is_invalid() as proof there is no pending overlapped
write and clears self.write_pending and returns, which can free OVERLAPPED and
write_buffer while the OS may still deliver completion; instead, when
handle.is_invalid() and self.write_pending is true, wait for the overlapped
event (via WaitForSingleObject on the OVERLAPPED event) or ensure pending I/O is
canceled (CancelIoEx) and completion drained before freeing buffers, rather than
returning early; also replace the unbounded GetOverlappedResult(..., true) wait
with a bounded wait (use WaitForSingleObject with a sensible timeout and then
GetOverlappedResult) so the thread cannot block forever—inspect and update the
logic around the handle.is_invalid() branch, the GetOverlappedResult call, and
the places that clear write_pending or free the write_buffer/OVERLAPPED to
ensure buffers remain alive until completion is processed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the relevant file
head -n 360 crates/engine/src/hid/windows.rs | tail -n 20

Repository: EffortlessMetrics/OpenRacing

Length of output: 873

---

🏁 Script executed:

# Get more context around the function
sed -n '330,370p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1802

---

🏁 Script executed:

# Find the function name and full implementation of cancel_and_drain
rg -A 30 "fn cancel_and_drain" crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1206

---

🏁 Script executed:

# Check if this struct implements Drop or is used in a destructor context
rg -B 10 "cancel_and_drain" crates/engine/src/hid/windows.rs | head -n 50

Repository: EffortlessMetrics/OpenRacing

Length of output: 1021

---

🏁 Script executed:

# Check for any timeout mechanisms in the windows.rs file
rg -i "timeout|deadline|duration|std::time" crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 992

---

🏁 Script executed:

# Check the full Drop implementation to see if there's any timeout handling
sed -n '/impl Drop for WindowsHidDevice/,/^}/p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 787

---

🏁 Script executed:

# Get the full cancel_and_drain implementation with all context
sed -n '/pub(crate) fn cancel_and_drain/,/^    }/p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1141

---

🏁 Script executed:

# Check if there are any recent changes or TODOs related to timeout in cancel_and_drain
rg -B 5 -A 15 "GetOverlappedResult.*true" crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1022

---

🏁 Script executed:

# Get the full Drop implementation for OverlappedWriteState
sed -n '/impl Drop for OverlappedWriteState/,/^}/p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1594

---

🏁 Script executed:

# Also check the struct definition to understand the fields available
sed -n '/struct OverlappedWriteState/,/^}/p' crates/engine/src/hid/windows.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1445

---

Add a finite timeout to the drain wait in cancel_and_drain().

Line 356 calls GetOverlappedResult(..., true), which blocks indefinitely until the overlapped operation completes. Because Windows does not guarantee that CancelIoEx() will successfully cancel the request, this can block the destructor thread indefinitely during shutdown. Wrap the GetOverlappedResult call with a timeout deadline and surface a hard failure (e.g., log a critical error or panic) if the deadline expires without the operation completing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/hid/windows.rs` around lines 353 - 356, In
cancel_and_drain(), replace the indefinite blocking GetOverlappedResult(handle,
&self.overlapped, &mut bytes_transferred, true) with a finite wait using the
OVERLAPPED event: use WaitForSingleObject(self.overlapped.hEvent, TIMEOUT_MS)
(or equivalent deadline logic) and if it returns signaled call
GetOverlappedResult(handle, &self.overlapped, &mut bytes_transferred, false); if
the wait times out, surface a hard failure (log critical/panic) indicating
CancelIoEx did not complete within the deadline so the destructor can fail fast.
Keep references to CancelIoEx, self.overlapped, handle and bytes_transferred
when implementing the change.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cd crates/plugins/tests && pwd && ls -la plugin_rt_tests.rs 2>/dev/null || echo "File not found at expected path"

Repository: EffortlessMetrics/OpenRacing

Length of output: 179

---

🏁 Script executed:

git ls-files | grep -E "plugin_rt_tests\.rs|plugin.*\.rs" | head -20

Repository: EffortlessMetrics/OpenRacing

Length of output: 1124

---

🏁 Script executed:

# Search for the test file and PluginFrame definition
fd "plugin_rt_tests" --type f

Repository: EffortlessMetrics/OpenRacing

Length of output: 111

---

🏁 Script executed:

cat -n crates/plugins/tests/plugin_rt_tests.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 2439

---

🏁 Script executed:

# Find the PluginFrame definition
rg "struct PluginFrame" --type rs -A 10

Repository: EffortlessMetrics/OpenRacing

Length of output: 98

---

🏁 Script executed:

# Also search in the racing_wheel_plugins crate
fd "racing_wheel_plugins" --type d

Repository: EffortlessMetrics/OpenRacing

Length of output: 54

---

🏁 Script executed:

# Find the PluginFrame definition with correct syntax
rg "struct PluginFrame" --type rust -A 10

Repository: EffortlessMetrics/OpenRacing

Length of output: 894

---

🏁 Script executed:

# Search in the plugins directory
find crates -name "*.rs" -type f | xargs grep -l "struct PluginFrame" | head -5

Repository: EffortlessMetrics/OpenRacing

Length of output: 117

---

🏁 Script executed:

# Read the full PluginFrame struct definition
sed -n '/pub struct PluginFrame/,/^}/p' crates/openracing-native-plugin/src/plugin.rs | head -30

Repository: EffortlessMetrics/OpenRacing

Length of output: 438

---

🏁 Script executed:

# Check for repr attribute and surrounding context
grep -B 5 "pub struct PluginFrame" crates/openracing-native-plugin/src/plugin.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 212

---

🏁 Script executed:

# Check the Cargo.toml to see what targets are supported
find crates -name "Cargo.toml" | head -1 | xargs cat | grep -A 10 "\[package\]"

Repository: EffortlessMetrics/OpenRacing

Length of output: 453

---

🏁 Script executed:

# Check if there's any existing target-specific code in the plugin module
grep -r "target_pointer_width\|target_os\|cfg!" crates/openracing-native-plugin/src/plugin.rs | head -10

Repository: EffortlessMetrics/OpenRacing

Length of output: 54

---

🏁 Script executed:

# Check the native plugin Cargo.toml
cat crates/openracing-native-plugin/Cargo.toml | head -30

Repository: EffortlessMetrics/OpenRacing

Length of output: 1108

---

🏁 Script executed:

# Check if there's any documentation about supported targets
grep -r "target" crates/openracing-native-plugin/Cargo.toml

Repository: EffortlessMetrics/OpenRacing

Length of output: 142

---

🏁 Script executed:

# Check if PluginFrame size is actually critical for RT safety
grep -B 5 -A 5 "PluginFrame" crates/openracing-native-plugin/src/plugin.rs | grep -E "comment|safety|real.time|RT|cache" | head -10

Repository: EffortlessMetrics/OpenRacing

Length of output: 108

---

🏁 Script executed:

# Check the plugin module documentation for RT constraints
head -50 crates/openracing-native-plugin/src/plugin.rs

Repository: EffortlessMetrics/OpenRacing

Length of output: 1250

---

Clean up inaccurate comment and remove leftover note.

Line 10's inline comment incorrectly describes the struct as f32*2 + u64 + u32 + padding when it actually contains f32*3 + u64 + u32*2 + padding. Line 11 is a leftover incomplete note. Update the comment to accurately describe the layout or remove it for clarity.

Suggested fix

 #[test]
 fn test_plugin_frame_layout_rt_safe() {
     // Verify that PluginFrame is repr(C) and has expected size/alignment for RT safety
-    assert_eq!(size_of::<PluginFrame>(), 32); // f32*2 + u64 + u32 + padding
-    // Wait, let's verify the actual size.
+    // PluginFrame layout: 3×f32 + u64 + 2×u32 + padding = 32 bytes on common targets
+    assert_eq!(size_of::<PluginFrame>(), 32);
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	assert_eq!(size_of::<PluginFrame>(), 32); // f32*2 + u64 + u32 + padding
	// Wait, let's verify the actual size.
	#[test]
	fn test_plugin_frame_layout_rt_safe() {
	// Verify that PluginFrame is repr(C) and has expected size/alignment for RT safety
	// PluginFrame layout: 3×f32 + u64 + 2×u32 + padding = 32 bytes on common targets
	assert_eq!(size_of::<PluginFrame>(), 32);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/plugins/tests/plugin_rt_tests.rs` around lines 10 - 11, Update the
inaccurate inline comment next to the size_of::<PluginFrame>() assertion: either
remove the leftover note and the incorrect layout comment, or replace it with
the correct field layout (PluginFrame contains f32*3 + u64 + u32*2 + padding) so
the comment matches the actual struct; the change should be made adjacent to the
size_of::<PluginFrame>() assertion in the test.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Redundant warning log at lines 79-82.

When real_port_failed is true, a warning is already emitted at line 72 ("Failed to initialize platform HID backend; falling back to virtual devices"). The additional warning at line 81 ("SERVICE DEGRADED: Running with virtual HID devices only") is largely redundant since the fallback scenario is already logged.

However, the second message emphasizes degraded service status which could be valuable for operational visibility. If keeping both, consider consolidating or differentiating the log levels.

♻️ Optional: Remove redundant warning or differentiate severity

Option 1 - Remove redundant warning:

-        // Apply fallback warning if both failed or if explicitly falling back
-        if real_port_failed {
-            warn!("SERVICE DEGRADED: Running with virtual HID devices only");
-        }

Option 2 - Use error level for degraded state to differentiate:

         // Apply fallback warning if both failed or if explicitly falling back
         if real_port_failed {
-            warn!("SERVICE DEGRADED: Running with virtual HID devices only");
+            error!("SERVICE DEGRADED: Running with virtual HID devices only");
         }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Initialize HID port
	// We attempt to initialize the real platform HID backend first.
	// If it fails or if virtual devices are explicitly requested, we fall back to VirtualHidPort.
	let mut real_port_failed = false;
	let hid_port: Arc<dyn HidPort> = if flags.enable_virtual_devices {
	info!("Virtual devices explicitly requested via feature flags");
	Self::create_virtual_port()?
	} else {
	match racing_wheel_engine::hid::create_hid_port() {
	Ok(port) => {
	info!("Platform HID backend initialized successfully");
	Arc::from(port)
	}
	Err(e) => {
	warn!(error = %e, "Failed to initialize platform HID backend; falling back to virtual devices");
	real_port_failed = true;
	Self::create_virtual_port()?
	}
	}
	};
	let hid_port = Arc::new(virtual_port);
	info!("HID port initialized with virtual device");
	// Apply fallback warning if both failed or if explicitly falling back
	if real_port_failed {
	warn!("SERVICE DEGRADED: Running with virtual HID devices only");
	}
	// Initialize HID port
	// We attempt to initialize the real platform HID backend first.
	// If it fails or if virtual devices are explicitly requested, we fall back to VirtualHidPort.
	let mut real_port_failed = false;
	let hid_port: Arc<dyn HidPort> = if flags.enable_virtual_devices {
	info!("Virtual devices explicitly requested via feature flags");
	Self::create_virtual_port()?
	} else {
	match racing_wheel_engine::hid::create_hid_port() {
	Ok(port) => {
	info!("Platform HID backend initialized successfully");
	Arc::from(port)
	}
	Err(e) => {
	warn!(error = %e, "Failed to initialize platform HID backend; falling back to virtual devices");
	real_port_failed = true;
	Self::create_virtual_port()?
	}
	}
	};

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/service.rs` around lines 58 - 82, The code logs a warning
twice when falling back to virtual HID devices: once in the Err branch of
racing_wheel_engine::hid::create_hid_port() and again using real_port_failed
after creating the virtual port; remove redundancy by either deleting the second
warn! that uses real_port_failed or change its level/message to clearly
differentiate (e.g., error or a distinct “SERVICE DEGRADED” metric) so you only
emit one meaningful operational log for the fallback in the Hid initialization
flow that involves flags.enable_virtual_devices, create_hid_port(),
create_virtual_port(), and the real_port_failed flag.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Keep the runtime and persisted defaults in sync.

Line 227 makes FeatureFlags::default().enable_virtual_devices true, while DevelopmentConfig::default() still leaves the same toggle false. That means the baseline behavior now depends on whether a caller comes in through SystemConfig::default() or through FeatureFlags::default(), which is a surprising constructor-level behavior change.

🛠 One way to remove the drift

 impl Default for FeatureFlags {
     fn default() -> Self {
+        let dev = DevelopmentConfig::default();
         Self {
             disable_realtime: false,
             force_ffb_mode: None,
-            enable_dev_features: false,
-            enable_debug_logging: false,
-            enable_virtual_devices: true, // Enable by default for safety/fallback
-            disable_safety_interlocks: false,
-            enable_plugin_dev_mode: false,
+            enable_dev_features: dev.enable_dev_features,
+            enable_debug_logging: dev.enable_debug_logging,
+            enable_virtual_devices: dev.enable_virtual_devices,
+            disable_safety_interlocks: dev.disable_safety_interlocks,
+            enable_plugin_dev_mode: dev.enable_plugin_dev_mode,
         }
     }
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	impl Default for FeatureFlags {
	fn default() -> Self {
	Self {
	disable_realtime: false,
	force_ffb_mode: None,
	enable_dev_features: false,
	enable_debug_logging: false,
	enable_virtual_devices: true, // Enable by default for safety/fallback
	disable_safety_interlocks: false,
	enable_plugin_dev_mode: false,
	}
	}
	}
	impl Default for FeatureFlags {
	fn default() -> Self {
	let dev = DevelopmentConfig::default();
	Self {
	disable_realtime: false,
	force_ffb_mode: None,
	enable_dev_features: dev.enable_dev_features,
	enable_debug_logging: dev.enable_debug_logging,
	enable_virtual_devices: dev.enable_virtual_devices,
	disable_safety_interlocks: dev.disable_safety_interlocks,
	enable_plugin_dev_mode: dev.enable_plugin_dev_mode,
	}
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/system_config.rs` around lines 220 - 232,
FeatureFlags::default() sets enable_virtual_devices = true but
DevelopmentConfig::default() still uses false, causing inconsistent behavior
depending on whether callers use FeatureFlags::default() or
SystemConfig::default(); update the persisted/default config to match the
runtime default (or vice versa) so both constructors are identical —
specifically, change DevelopmentConfig::default() (the struct that holds
enable_virtual_devices in persisted settings) to set enable_virtual_devices =
true (or alter FeatureFlags::default() if the intended baseline is false) and
ensure any comments/tests referencing the previous value are updated to reflect
the chosen consistent default.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

The Windows remap is unreachable for real callers.

Line 18 returns on every non-empty game_path, and the main service path passes the actual install directory into write_config. That means all of the new resolve_game_path(..., "Documents/...") call sites still resolve under <install>/Documents/... in real usage, so this fix only works in temp-dir tests. Split the install root from a test-only Documents override instead of using game_path emptiness as the switch.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 16 - 19, The early
return in resolve_game_path (and usages in write_config) makes the Windows
"Documents" remap unreachable because real callers pass the install directory as
game_path; instead, change the API to separate install_root from a test-only
documents_override (e.g., add an Option<PathBuf> parameter or a distinct
documents_root) and update resolve_game_path to: always treat game_path as the
install root, and only use the documents_override when relative_path targets
"Documents/..." (apply the Windows remap logic when
documents_override.is_some()), then update write_config and all
resolve_game_path call sites to provide the actual install directory as
game_path and the optional test Documents override where appropriate so the
remap works in tests without breaking real usage.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/ui/src/commands.rs:357 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:189 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:192 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:211 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:608 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:217 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:744 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/linux.rs:1253 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/linux.rs:1282 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/hid/mod.rs:203 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/compat_impl.rs:61 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:189 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:192 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:211 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/integration-tests/src/common.rs (1)

23-30: ⚠️ Potential issue | 🟠 Major

The new torque-history helper is inspecting the wrong device instance.

Line 148 constructs a WheelService with its own internal racing_wheel_engine::VirtualHidPort, but Lines 23-84 record torque history on a separate crates/integration-tests/src/common.rs::VirtualDevice stored only in TestHarness::virtual_devices. The actual force-feedback writes land in the engine-side VirtualDevice ring buffer shown in crates/engine/src/device.rs:240-271, so verify_torque_ramp_to_zero() cannot observe the service's real torque output. Even if the service loop is started elsewhere, this mismatch remains until the harness and service share the same virtual port/device.

Also applies to: 58-84, 136-149

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/integration-tests/src/common.rs` around lines 23 - 30, The test is
reading torque from integration-tests::common::VirtualDevice stored in
TestHarness::virtual_devices while the WheelService writes force-feedback into
its own racing_wheel_engine::VirtualHidPort/VirtualDevice, so
verify_torque_ramp_to_zero() inspects the wrong instance; fix by making the
harness and service share the same virtual port/device: construct the
racing_wheel_engine::VirtualHidPort (and its VirtualDevice) in the TestHarness
and pass that instance into WheelService (or alternatively expose the
engine-side device/ring buffer via WheelService so tests can read the same
torque_history), then update verify_torque_ramp_to_zero() and other tests (lines
noted around VirtualDevice, TestHarness::virtual_devices, WheelService, and
verify_torque_ramp_to_zero) to read the shared device's torque_history.

♻️ Duplicate comments (5)

crates/service/src/game_auto_configure.rs (2)

3-4: ⚠️ Potential issue | 🟡 Minor

Replace the machine-local ADR link.

The file:///h:/Code/... URI is machine-specific and will not resolve for other contributors, in CI, or in generated rustdoc. Use a repository-relative path (e.g., ../../docs/adr/0008-game-auto-configure-telemetry-bridge.md) or plain text reference (ADR-0008).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/game_auto_configure.rs` around lines 3 - 4, The
crate-level doc comment contains a machine-local file URI
("file:///h:/Code/...") that won't resolve for others; update the module doc in
game_auto_configure.rs to reference the ADR using a repository-relative path
(for example ../../docs/adr/0008-game-auto-configure-telemetry-bridge.md) or a
short plain-text reference like "ADR-0008: Game Auto-Configure" so rustdoc and
CI can resolve the reference consistently.

---

200-205: ⚠️ Potential issue | 🟠 Major

Probing all 24 Windows drive letters synchronously creates stall risk.

Path::exists() on offline network or removable drives can block indefinitely. The loop iterates C–Z (24 drives), and install_path_roots() is called inside the for rel_path loop at line 146, multiplying the stall risk per install path. Use the Windows GetLogicalDrives() API to enumerate only mounted drives, or cache the result once per detection cycle.

🛠️ Suggested approach using GetLogicalDrives

+#[cfg(target_os = "windows")]
+fn install_path_roots() -> Vec<PathBuf> {
+    use std::os::windows::ffi::OsStringExt;
+    // GetLogicalDrives returns a bitmask of available drives (bit 0 = A, bit 2 = C, etc.)
+    let mask = unsafe { windows_sys::Win32::Storage::FileSystem::GetLogicalDrives() };
+    let mut roots = Vec::new();
+    for i in 2..26 {  // C (2) through Z (25)
+        if (mask & (1 << i)) != 0 {
+            let letter = (b'A' + i) as char;
+            roots.push(PathBuf::from(format!("{}:\\", letter)));
+        }
+    }
+    roots
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/game_auto_configure.rs` around lines 200 - 205, The
synchronous loop that probes C..=Z with Path::exists in the install_path_roots()
routine can block on offline/removable drives; replace it by enumerating mounted
drives via the Windows GetLogicalDrives() API (using the windows or winapi
crate) or at minimum cache the discovered roots once per detection cycle and
reuse that cache instead of re-running the C..=Z probe for every call. Locate
the drive-probing block (the for letter in b'C'..=b'Z' loop inside
install_path_roots()) and change it to call a helper that returns only mounted
drive letters from GetLogicalDrives(), or move the current probe out of the
inner rel_path loop so results are computed once and reused.

crates/telemetry-config-writers/src/lib.rs (2)

22-29: ⚠️ Potential issue | 🟠 Major

Don’t guess the Windows Documents folder from USERPROFILE.

Redirected, OneDrive-managed, and policy-overridden profiles make %USERPROFILE%\Documents an unreliable target, so this can still read or write the wrong config path even after the control-flow issue is fixed. Use the actual Documents known folder and keep the env-var path only as a fallback. As per coding guidelines, "Avoid OS-specific assumptions unless the module is platform-specific; use platform-specific APIs (MMCSS on Windows, SCHED_FIFO on Linux, thread_policy_set on macOS) only in designated modules".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 22 - 29, The current
cfg(windows) branch guesses the Documents folder from USERPROFILE by using
relative_path.strip_prefix("Documents/") and joining to
PathBuf::from(user_profile), which is unreliable for OneDrive-redirected or
policy-overridden folders; replace this logic in the cfg(windows) block to
resolve the actual known Documents folder via the Windows Known Folder API
(e.g., call SHGetKnownFolderPath / use the windows or winapi crate to obtain
FOLDERID_Documents) and only fall back to the USERPROFILE/Documents heuristic if
the Known Folder API call fails, keeping the rest of the function (the
strip_prefix handling and returning path.join(stripped.replace('/', "\\")))
intact.

---

14-31: ⚠️ Potential issue | 🔴 Critical

resolve_game_path() still never reaches the Windows remap for real inputs.

Line 18 returns on any non-. game_path, so the Documents/... branch only runs for "." or empty callers. That means the migrated writers below still resolve Documents/... under the caller-provided base path, and the new path_resolution_tests::test_resolve_game_path already expects the opposite behavior on Windows. Split the install root from an optional Documents override instead of using game_path for both.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/telemetry-config-writers/src/lib.rs` around lines 14 - 31, The
resolve_game_path function currently returns early whenever game_path is
non-empty or not ".", preventing the Windows "Documents/..." remap from ever
running for real inputs; change the logic to treat the caller-provided install
root separately from an optional Documents override so that when relative_path
starts with "Documents/" you ignore the install root and remap to
USERPROFILE/Documents on Windows (preserving behavior for tests that pass "." or
empty), i.e., split the "install root" (the base to use for normal relative
paths) from a special-case Documents override and implement the Windows-only
remap branch to run when relative_path.strip_prefix("Documents/") is Some, using
std::env::var_os("USERPROFILE") and joining the stripped remainder after
converting '/' to '\' on Windows.

crates/engine/src/hid/windows.rs (1)

339-362: ⚠️ Potential issue | 🔴 Critical

Confirm completion before clearing write_pending.

The handle.is_invalid() fast-path and the ignored GetOverlappedResult(..., true) result both let this state report “drained” without proving that the kernel has finished with self.overlapped and write_buffer. That recreates the use-after-free window during Drop, and the current bWait = TRUE call can also hang shutdown indefinitely. Wait on self.event_handle with a finite timeout, only clear write_pending after the event is signaled, and only call GetOverlappedResult(..., false) when the file handle is still valid.

For Win32 overlapped I/O, after CancelIoEx or CloseHandle, must the OVERLAPPED structure and backing buffers remain valid until completion is signaled, and does GetOverlappedResult(handle, overlapped, ..., TRUE) wait indefinitely for that completion?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/hid/windows.rs` around lines 339 - 362, The fast-path that
returns when handle.is_invalid() and the current use of GetOverlappedResult(...,
TRUE) are unsafe: ensure the OVERLAPPED and write_buffer remain valid until the
kernel signals completion by waiting on self.event_handle with a finite timeout
(instead of returning immediately when handle.is_invalid()); only clear
self.write_pending after the event is signaled (or the timeout elapses) and only
call GetOverlappedResult(handle, &self.overlapped, ..., false) when the handle
is still valid; keep CancelIoEx(handle, Some(&self.overlapped)) but replace the
blocking TRUE wait with an event wait+finite timeout and then a non-blocking
GetOverlappedResult (wait=false) if handle.is_valid(), otherwise rely on the
event signal to ensure resources are no longer referenced before dropping.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 617-633: The current heuristic uses TEST_LINE (first #[cfg(test)])
and PRE_TEST_UNWRAPS to ignore unwraps after that point, which misses production
unwraps that appear after an inline test module; fix by making the exemption
scope-aware or fail closed: either (A) replace the single-cutoff logic with
scanning all #[cfg(test)] occurrences and computing their explicit module/braces
ranges (e.g., iterate each TEST_LINE, find the corresponding opening "mod" block
and match braces to get start/end lines, then exclude .unwrap() only within
those ranges) using the same variables REAL_VIOLATIONS, TEST_LINE,
PRE_TEST_UNWRAPS as anchors, or (B) until scope-aware parsing is implemented,
change the logic to treat any file containing any #[cfg(test)] as not eligible
for the pre-test exemption (i.e., mark any .unwrap() as a REAL_VIOLATION when
TEST_LINE is non-empty) so we fail closed.

In `@crates/engine/src/hid/windows.rs`:
- Around line 4045-4057: Change the test `test_overlapped_write_state` to be a
Result-returning test instead of calling `expect()`: update the signature to
return Result<(), E> (e.g., `Result<(), Box<dyn std::error::Error>>` or an
appropriate error type), replace the `OverlappedWriteState::new().expect("...")`
call with the `?` operator on `OverlappedWriteState::new()`, keep the assertions
(e.g., `assert!(!state.write_pending.load(...))`) and
`state.reset_overlapped()`, and return Ok(()) at the end so the test uses `?`
rather than `expect()`; reference symbols: `test_overlapped_write_state`,
`OverlappedWriteState::new`, `reset_overlapped`, and `write_pending`.

In `@crates/integration-tests/tests/integration_tests.rs`:
- Around line 36-44: The conditional that checks gates::ci_gates_enabled()
inside acceptance_subset_timeout() is redundant because both branches return
Duration::from_secs(180); remove the if/else entirely and replace it with a
single unconditional return of Duration::from_secs(180), keeping the function
name (acceptance_subset_timeout) and the Duration::from_secs(180) expression to
locate the change.

In `@crates/openracing-native-plugin/src/spsc.rs`:
- Around line 341-378: Change the two tests (test_spsc_frame_size_mismatch and
test_spsc_buffer_size_mismatch) to be Result-returning tests (e.g., change
signature to return Result<(), impl Error>), remove all uses of unwrap_err(),
expect(), and panic!, and instead capture the Err from writer.write(...) and
reader.read(...) with pattern matching (match or if let Err(e) = ...), assert
the error is the expected NativePluginError::FrameSizeMismatch or
NativePluginError::BufferSizeMismatch and assert_eq! on the expected and actual
fields, then return Ok(()) from the test; use the SpscChannel::new,
writer.write, reader.read, and the NativePluginError variants to locate and
update the code.

In `@crates/service/src/ipc.rs`:
- Around line 1208-1235: The test test_get_device_status_telemetry_conversion
must be made into a Result-returning test (e.g., -> Result<(), Box<dyn
std::error::Error>>) and replace .expect() calls on service.get_device_status()
and status.telemetry with the ? operator and proper pattern matching so no
unwrap/expect is used; then add a new unit test for the safe_clamp function that
exercises edge cases (NaN, f32::INFINITY, f32::NEG_INFINITY, values just
above/below i32::MAX/i32::MIN and typical extremes) validating the returned i32
bounds and that NaN is handled deterministically, using assert_eq!/assert_ne! on
the converted values and returning Result as well to follow the no-unwrap
guideline.
- Around line 700-709: The current clamping uses f32 bounds (i32::MIN as f32 /
i32::MAX as f32) which are not exactly representable and yield imprecise ranges
for wheel_angle_mdeg and wheel_speed_mrad_s; update the conversion to perform
the clamp in f64 (or otherwise with types that can exactly represent i32
extremal values) using openracing_hid_common::math::safe_clamp (or a new
safe_clamp_f64 variant) with i32::MIN as f64 and i32::MAX as f64, then
round/saturate to i32 when casting; alternatively add precise i32 bounds
constants in openracing_hid_common::math and use those in the wheel_angle_mdeg
and wheel_speed_mrad_s conversion sites to ensure exact clamping.

In `@crates/service/src/service.rs`:
- Around line 59-76: The current logic treats enable_virtual_devices as "force
virtual", bypassing racing_wheel_engine::hid::create_hid_port() whenever it's
true; change it so the code always attempts create_hid_port() first and only
calls Self::create_virtual_port() if create_hid_port() returns Err (or,
alternatively, add a separate force_virtual_devices flag and only skip
create_hid_port() when that new flag is set); update the initialization in
WheelService::new()/new_with_flags and FeatureFlags::default() accordingly so
the default feature flags do not force virtual devices, and keep references to
create_hid_port(), create_virtual_port(), enable_virtual_devices,
WheelService::new()/new_with_flags, and FeatureFlags::default() to locate the
change.

---

Outside diff comments:
In `@crates/integration-tests/src/common.rs`:
- Around line 23-30: The test is reading torque from
integration-tests::common::VirtualDevice stored in TestHarness::virtual_devices
while the WheelService writes force-feedback into its own
racing_wheel_engine::VirtualHidPort/VirtualDevice, so
verify_torque_ramp_to_zero() inspects the wrong instance; fix by making the
harness and service share the same virtual port/device: construct the
racing_wheel_engine::VirtualHidPort (and its VirtualDevice) in the TestHarness
and pass that instance into WheelService (or alternatively expose the
engine-side device/ring buffer via WheelService so tests can read the same
torque_history), then update verify_torque_ramp_to_zero() and other tests (lines
noted around VirtualDevice, TestHarness::virtual_devices, WheelService, and
verify_torque_ramp_to_zero) to read the shared device's torque_history.

---

Duplicate comments:
In `@crates/engine/src/hid/windows.rs`:
- Around line 339-362: The fast-path that returns when handle.is_invalid() and
the current use of GetOverlappedResult(..., TRUE) are unsafe: ensure the
OVERLAPPED and write_buffer remain valid until the kernel signals completion by
waiting on self.event_handle with a finite timeout (instead of returning
immediately when handle.is_invalid()); only clear self.write_pending after the
event is signaled (or the timeout elapses) and only call
GetOverlappedResult(handle, &self.overlapped, ..., false) when the handle is
still valid; keep CancelIoEx(handle, Some(&self.overlapped)) but replace the
blocking TRUE wait with an event wait+finite timeout and then a non-blocking
GetOverlappedResult (wait=false) if handle.is_valid(), otherwise rely on the
event signal to ensure resources are no longer referenced before dropping.

In `@crates/service/src/game_auto_configure.rs`:
- Around line 3-4: The crate-level doc comment contains a machine-local file URI
("file:///h:/Code/...") that won't resolve for others; update the module doc in
game_auto_configure.rs to reference the ADR using a repository-relative path
(for example ../../docs/adr/0008-game-auto-configure-telemetry-bridge.md) or a
short plain-text reference like "ADR-0008: Game Auto-Configure" so rustdoc and
CI can resolve the reference consistently.
- Around line 200-205: The synchronous loop that probes C..=Z with Path::exists
in the install_path_roots() routine can block on offline/removable drives;
replace it by enumerating mounted drives via the Windows GetLogicalDrives() API
(using the windows or winapi crate) or at minimum cache the discovered roots
once per detection cycle and reuse that cache instead of re-running the C..=Z
probe for every call. Locate the drive-probing block (the for letter in
b'C'..=b'Z' loop inside install_path_roots()) and change it to call a helper
that returns only mounted drive letters from GetLogicalDrives(), or move the
current probe out of the inner rel_path loop so results are computed once and
reused.

In `@crates/telemetry-config-writers/src/lib.rs`:
- Around line 22-29: The current cfg(windows) branch guesses the Documents
folder from USERPROFILE by using relative_path.strip_prefix("Documents/") and
joining to PathBuf::from(user_profile), which is unreliable for
OneDrive-redirected or policy-overridden folders; replace this logic in the
cfg(windows) block to resolve the actual known Documents folder via the Windows
Known Folder API (e.g., call SHGetKnownFolderPath / use the windows or winapi
crate to obtain FOLDERID_Documents) and only fall back to the
USERPROFILE/Documents heuristic if the Known Folder API call fails, keeping the
rest of the function (the strip_prefix handling and returning
path.join(stripped.replace('/', "\\"))) intact.
- Around line 14-31: The resolve_game_path function currently returns early
whenever game_path is non-empty or not ".", preventing the Windows
"Documents/..." remap from ever running for real inputs; change the logic to
treat the caller-provided install root separately from an optional Documents
override so that when relative_path starts with "Documents/" you ignore the
install root and remap to USERPROFILE/Documents on Windows (preserving behavior
for tests that pass "." or empty), i.e., split the "install root" (the base to
use for normal relative paths) from a special-case Documents override and
implement the Windows-only remap branch to run when
relative_path.strip_prefix("Documents/") is Some, using
std::env::var_os("USERPROFILE") and joining the stripped remainder after
converting '/' to '\' on Windows.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3da9e484-9070-427d-a800-d91ecc9aba62

📥 Commits

Reviewing files that changed from the base of the PR and between 80f1c61 and d3f161c.

📒 Files selected for processing (18)

• .github/workflows/ci.yml
• crates/engine/src/hid/windows.rs
• crates/engine/src/hid/windows_property_tests.rs
• crates/hid-moza-protocol/src/protocol.rs
• crates/integration-tests/src/common.rs
• crates/integration-tests/tests/integration_tests.rs
• crates/openracing-native-plugin/src/spsc.rs
• crates/service/src/daemon.rs
• crates/service/src/game_auto_configure.rs
• crates/service/src/ipc.rs
• crates/service/src/service.rs
• crates/service/src/service_tests.rs
• crates/service/tests/lifecycle_tests.rs
• crates/service/tests/service_hardening_tests.rs
• crates/service/tests/service_integration_tests.rs
• crates/service/tests/service_ipc_deep_tests.rs
• crates/service/tests/service_lifecycle_hardening_tests.rs
• crates/telemetry-config-writers/src/lib.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't use the first #[cfg(test)] as a file-wide cutoff.

Lines 621-630 only inspect unwrap() before the first #[cfg(test)]. Rust items are not ordered that way, so a file can have an inline test module/helper and then later return to production code; any production unwrap() after that point now passes this gate silently. Please make the exemption scope-aware, or fail closed until it is.

This reproduces the current shell logic against a synthetic Rust file. Expected result: it prints that the production unwrap() was missed.

#!/bin/bash
set -euo pipefail

tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT

cat > "$tmp" <<'EOF'
fn prod_before_tests() {}

#[cfg(test)]
mod tests {
    #[test]
    fn test_only_unwrap() {
        Some(1).unwrap();
    }
}

fn prod_after_tests() {
    Some(2).unwrap();
}
EOF

UNWRAP_VIOLATIONS=$(grep -l -P '^\s*(?!.*///).*\.unwrap\(\)' "$tmp" 2>/dev/null || true)

REAL_VIOLATIONS=""
for file in $UNWRAP_VIOLATIONS; do
  TEST_LINE=$(grep -n '#\[cfg(test)\]' "$file" 2>/dev/null | head -1 | cut -d: -f1)
  if [ -n "$TEST_LINE" ]; then
    PRE_TEST_UNWRAPS=$(head -n "$((TEST_LINE - 1))" "$file" | grep -c '\.unwrap()' 2>/dev/null || true)
    if [ "$PRE_TEST_UNWRAPS" -gt 0 ]; then
      REAL_VIOLATIONS="$REAL_VIOLATIONS $file"
    fi
  else
    REAL_VIOLATIONS="$REAL_VIOLATIONS $file"
  fi
done

if [ -z "$REAL_VIOLATIONS" ]; then
  echo "BUG VERIFIED: production unwrap after #[cfg(test)] is missed"
else
  echo "Unexpected result: heuristic caught the sample"
  exit 1
fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 617 - 633, The current heuristic uses
TEST_LINE (first #[cfg(test)]) and PRE_TEST_UNWRAPS to ignore unwraps after that
point, which misses production unwraps that appear after an inline test module;
fix by making the exemption scope-aware or fail closed: either (A) replace the
single-cutoff logic with scanning all #[cfg(test)] occurrences and computing
their explicit module/braces ranges (e.g., iterate each TEST_LINE, find the
corresponding opening "mod" block and match braces to get start/end lines, then
exclude .unwrap() only within those ranges) using the same variables
REAL_VIOLATIONS, TEST_LINE, PRE_TEST_UNWRAPS as anchors, or (B) until
scope-aware parsing is implemented, change the logic to treat any file
containing any #[cfg(test)] as not eligible for the pre-test exemption (i.e.,
mark any .unwrap() as a REAL_VIOLATION when TEST_LINE is non-empty) so we fail
closed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Replace expect() with a Result-returning test.

This reintroduces expect() in test code. Make the test return Result and use ? instead.

♻️ Proposed fix

 #[test]
-fn test_overlapped_write_state() {
-    let mut state =
-        OverlappedWriteState::new().expect("Failed to create overlapped write state");
+fn test_overlapped_write_state() -> Result<(), Box<dyn std::error::Error>> {
+    let mut state = OverlappedWriteState::new()?;
     assert!(
         !state.write_pending.load(Ordering::SeqCst),
         "Should not be pending initially"
     );
     state.reset_overlapped();
 
     // Validates the struct can be safely dropped without panic when not pending
     drop(state);
+    Ok(())
 }

As per coding guidelines, "Test code must not use unwrap() or expect(). Prefer Result-returning tests (e.g., #[test] fn foo() -> Result<(), Error>), explicit assertions (assert!(result.is_ok()), assert_eq!), or test helper macros".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	#[test]
	fn test_overlapped_write_state() {
	let mut state =
	OverlappedWriteState::new().expect("Failed to create overlapped write state");
	assert!(
	!state.write_pending.load(Ordering::SeqCst),
	"Should not be pending initially"
	);
	state.reset_overlapped();
	// Validates the struct can be safely dropped without panic when not pending
	drop(state);
	}
	#[test]
	fn test_overlapped_write_state() -> Result<(), Box<dyn std::error::Error>> {
	let mut state = OverlappedWriteState::new()?;
	assert!(
	!state.write_pending.load(Ordering::SeqCst),
	"Should not be pending initially"
	);
	state.reset_overlapped();
	// Validates the struct can be safely dropped without panic when not pending
	drop(state);
	Ok(())
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/engine/src/hid/windows.rs` around lines 4045 - 4057, Change the test
`test_overlapped_write_state` to be a Result-returning test instead of calling
`expect()`: update the signature to return Result<(), E> (e.g., `Result<(),
Box<dyn std::error::Error>>` or an appropriate error type), replace the
`OverlappedWriteState::new().expect("...")` call with the `?` operator on
`OverlappedWriteState::new()`, keep the assertions (e.g.,
`assert!(!state.write_pending.load(...))`) and `state.reset_overlapped()`, and
return Ok(()) at the end so the test uses `?` rather than `expect()`; reference
symbols: `test_overlapped_write_state`, `OverlappedWriteState::new`,
`reset_overlapped`, and `write_pending`.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Simplify redundant CI conditional in acceptance_subset_timeout().

Line 36 currently branches on gates::ci_gates_enabled(), but both branches return Duration::from_secs(180). You can collapse this to a single return to remove dead branching.

Proposed simplification

 fn acceptance_subset_timeout() -> Duration {
-    if gates::ci_gates_enabled() {
-        // 180s gives headroom for slow CI runners while still detecting
-        // genuine hangs. The previous 60/90s limits caused spurious timeouts
-        // when sysinfo::System::new_all() scanned every host process and
-        // service initialization was slower under CI resource contention.
-        Duration::from_secs(180)
-    } else {
-        Duration::from_secs(180)
-    }
+    // 180s gives headroom for slow CI runners while still detecting
+    // genuine hangs. The previous 60/90s limits caused spurious timeouts
+    // when sysinfo::System::new_all() scanned every host process and
+    // service initialization was slower under CI resource contention.
+    Duration::from_secs(180)
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if gates::ci_gates_enabled() {
	Duration::from_secs(60)
	// 180s gives headroom for slow CI runners while still detecting
	// genuine hangs. The previous 60/90s limits caused spurious timeouts
	// when sysinfo::System::new_all() scanned every host process and
	// service initialization was slower under CI resource contention.
	Duration::from_secs(180)
	} else {
	Duration::from_secs(180)
	}
	fn acceptance_subset_timeout() -> Duration {
	// 180s gives headroom for slow CI runners while still detecting
	// genuine hangs. The previous 60/90s limits caused spurious timeouts
	// when sysinfo::System::new_all() scanned every host process and
	// service initialization was slower under CI resource contention.
	Duration::from_secs(180)
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/integration-tests/tests/integration_tests.rs` around lines 36 - 44,
The conditional that checks gates::ci_gates_enabled() inside
acceptance_subset_timeout() is redundant because both branches return
Duration::from_secs(180); remove the if/else entirely and replace it with a
single unconditional return of Duration::from_secs(180), keeping the function
name (acceptance_subset_timeout) and the Duration::from_secs(180) expression to
locate the change.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test code uses unwrap_err(), expect(), and panic!() which violates coding guidelines.

The new tests use unwrap_err() (lines 348, 370), expect() (lines 344, 361, 366), and panic!() (lines 354, 376). Per coding guidelines, test code should use Result-returning tests or explicit assertions instead.

Proposed fix using Result-returning tests with assert_matches pattern

 #[test]
-fn test_spsc_frame_size_mismatch() {
+fn test_spsc_frame_size_mismatch() -> Result<(), NativePluginError> {
     let frame_size = 64;
-    let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
+    let channel = SpscChannel::new(frame_size)?;
     let writer = channel.writer();

     let oversized_frame = vec![0x42u8; frame_size + 1];
-    let err = writer.write(&oversized_frame).unwrap_err();
-    match err {
-        NativePluginError::FrameSizeMismatch { expected, actual } => {
-            assert_eq!(expected, 64);
-            assert_eq!(actual, 65);
-        }
-        _ => panic!("Expected FrameSizeMismatch"),
-    }
+    let result = writer.write(&oversized_frame);
+    assert!(matches!(
+        result,
+        Err(NativePluginError::FrameSizeMismatch { expected: 64, actual: 65 })
+    ));
+    Ok(())
 }

 #[test]
-fn test_spsc_buffer_size_mismatch() {
+fn test_spsc_buffer_size_mismatch() -> Result<(), NativePluginError> {
     let frame_size = 64;
-    let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
+    let channel = SpscChannel::new(frame_size)?;

     let writer = channel.writer();
-    writer
-        .write(&vec![0x42u8; frame_size])
-        .expect("Failed to write");
+    writer.write(&vec![0x42u8; frame_size])?;

     let reader = channel.reader();
     let mut undersized_buffer = vec![0u8; frame_size - 1];
-    let err = reader.read(&mut undersized_buffer).unwrap_err();
-    match err {
-        NativePluginError::BufferSizeMismatch { expected, actual } => {
-            assert_eq!(expected, 64);
-            assert_eq!(actual, 63);
-        }
-        _ => panic!("Expected BufferSizeMismatch"),
-    }
+    let result = reader.read(&mut undersized_buffer);
+    assert!(matches!(
+        result,
+        Err(NativePluginError::BufferSizeMismatch { expected: 64, actual: 63 })
+    ));
+    Ok(())
 }

As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests (e.g., #[test] fn foo() -> Result<(), Error>), explicit assertions (assert!(result.is_ok()), assert_eq!), or test helper macros"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	#[test]
	fn test_spsc_frame_size_mismatch() {
	let frame_size = 64;
	let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
	let writer = channel.writer();
	let oversized_frame = vec![0x42u8; frame_size + 1];
	let err = writer.write(&oversized_frame).unwrap_err();
	match err {
	NativePluginError::FrameSizeMismatch { expected, actual } => {
	assert_eq!(expected, 64);
	assert_eq!(actual, 65);
	}
	_ => panic!("Expected FrameSizeMismatch"),
	}
	}
	#[test]
	fn test_spsc_buffer_size_mismatch() {
	let frame_size = 64;
	let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
	let writer = channel.writer();
	writer
	.write(&vec![0x42u8; frame_size])
	.expect("Failed to write");
	let reader = channel.reader();
	let mut undersized_buffer = vec![0u8; frame_size - 1];
	let err = reader.read(&mut undersized_buffer).unwrap_err();
	match err {
	NativePluginError::BufferSizeMismatch { expected, actual } => {
	assert_eq!(expected, 64);
	assert_eq!(actual, 63);
	}
	_ => panic!("Expected BufferSizeMismatch"),
	}
	}
	#[test]
	fn test_spsc_frame_size_mismatch() -> Result<(), NativePluginError> {
	let frame_size = 64;
	let channel = SpscChannel::new(frame_size)?;
	let writer = channel.writer();
	let oversized_frame = vec![0x42u8; frame_size + 1];
	let result = writer.write(&oversized_frame);
	assert!(matches!(
	result,
	Err(NativePluginError::FrameSizeMismatch { expected: 64, actual: 65 })
	));
	Ok(())
	}
	#[test]
	fn test_spsc_buffer_size_mismatch() -> Result<(), NativePluginError> {
	let frame_size = 64;
	let channel = SpscChannel::new(frame_size)?;
	let writer = channel.writer();
	writer.write(&vec![0x42u8; frame_size])?;
	let reader = channel.reader();
	let mut undersized_buffer = vec![0u8; frame_size - 1];
	let result = reader.read(&mut undersized_buffer);
	assert!(matches!(
	result,
	Err(NativePluginError::BufferSizeMismatch { expected: 64, actual: 63 })
	));
	Ok(())
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/openracing-native-plugin/src/spsc.rs` around lines 341 - 378, Change
the two tests (test_spsc_frame_size_mismatch and test_spsc_buffer_size_mismatch)
to be Result-returning tests (e.g., change signature to return Result<(), impl
Error>), remove all uses of unwrap_err(), expect(), and panic!, and instead
capture the Err from writer.write(...) and reader.read(...) with pattern
matching (match or if let Err(e) = ...), assert the error is the expected
NativePluginError::FrameSizeMismatch or NativePluginError::BufferSizeMismatch
and assert_eq! on the expected and actual fields, then return Ok(()) from the
test; use the SpscChannel::new, writer.write, reader.read, and the
NativePluginError variants to locate and update the code.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Imprecise clamping bounds due to f32 precision loss.

i32::MAX as f32 rounds up to 2147483648.0 (exceeds i32::MAX by 1). While Rust 1.45+ saturates the subsequent as i32 cast (avoiding UB), the clamping range is semantically incorrect—values in [2147483647, 2147483648.0] all map to i32::MAX after saturation, which is acceptable but not precise.

For stricter correctness, use bounds that are exactly representable in both types:

🔧 Suggested fix for precise bounds

 wheel_angle_mdeg: openracing_hid_common::math::safe_clamp(
     t.wheel_angle_deg * 1000.0,
-    i32::MIN as f32,
-    i32::MAX as f32,
+    -2_147_483_648.0, // i32::MIN, exactly representable
+    2_147_483_520.0,  // largest f32 <= i32::MAX
 ) as i32,
 wheel_speed_mrad_s: openracing_hid_common::math::safe_clamp(
     t.wheel_speed_rad_s * 1000.0,
-    i32::MIN as f32,
-    i32::MAX as f32,
+    -2_147_483_648.0,
+    2_147_483_520.0,
 ) as i32,

Alternatively, consider defining these constants in openracing_hid_common::math for reuse across all conversion sites mentioned in the AI summary.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	wheel_angle_mdeg: openracing_hid_common::math::safe_clamp(
	t.wheel_angle_deg * 1000.0,
	i32::MIN as f32,
	i32::MAX as f32,
	) as i32,
	wheel_speed_mrad_s: openracing_hid_common::math::safe_clamp(
	t.wheel_speed_rad_s * 1000.0,
	i32::MIN as f32,
	i32::MAX as f32,
	) as i32,
	wheel_angle_mdeg: openracing_hid_common::math::safe_clamp(
	t.wheel_angle_deg * 1000.0,
	-2_147_483_648.0, // i32::MIN, exactly representable
	2_147_483_520.0, // largest f32 <= i32::MAX
	) as i32,
	wheel_speed_mrad_s: openracing_hid_common::math::safe_clamp(
	t.wheel_speed_rad_s * 1000.0,
	-2_147_483_648.0,
	2_147_483_520.0,
	) as i32,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/ipc.rs` around lines 700 - 709, The current clamping uses
f32 bounds (i32::MIN as f32 / i32::MAX as f32) which are not exactly
representable and yield imprecise ranges for wheel_angle_mdeg and
wheel_speed_mrad_s; update the conversion to perform the clamp in f64 (or
otherwise with types that can exactly represent i32 extremal values) using
openracing_hid_common::math::safe_clamp (or a new safe_clamp_f64 variant) with
i32::MIN as f64 and i32::MAX as f64, then round/saturate to i32 when casting;
alternatively add precise i32 bounds constants in openracing_hid_common::math
and use those in the wheel_angle_mdeg and wheel_speed_mrad_s conversion sites to
ensure exact clamping.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test uses expect() which violates coding guidelines; also lacks edge-case coverage.

The test calls .expect() on lines 1227 and 1231, violating the project rule that test code must not use unwrap() or expect(). Additionally, the mock returns 0.0 values, so this test doesn't exercise the edge cases (NaN, extreme values) that safe_clamp is designed to handle.

🧪 Proposed fix: Result-returning test with edge-case coverage

 #[tokio::test]
-async fn test_get_device_status_telemetry_conversion() {
+async fn test_get_device_status_telemetry_conversion() -> Result<(), Box<dyn std::error::Error>> {
     let (tx, _) = broadcast::channel(10);
     let service = WheelServiceImpl {
         device_service: Arc::new(DeviceService),
         profile_service: Arc::new(ProfileService),
         game_service: Arc::new(GameService),
         safety_service: Arc::new(SafetyService),
         health_broadcaster: tx,
         connected_clients: Arc::new(RwLock::new(HashMap::new())),
     };

     let request = Request::new(DeviceId {
         id: "test-device-1".to_string(),
     });

     let response = service
         .get_device_status(request)
-        .await
-        .expect("Failed to get device status");
-    
-    let status = response.into_inner();
-    
-    let telemetry = status.telemetry.expect("Expected telemetry data");
+        .await?;
+
+    let status = response.into_inner();
+
+    assert!(status.telemetry.is_some(), "Expected telemetry data");
+    let telemetry = status.telemetry.ok_or("missing telemetry")?;
     // Ensure conversions happened safely and didn't panic.
     assert_eq!(telemetry.wheel_angle_mdeg, 0);
     assert_eq!(telemetry.wheel_speed_mrad_s, 0);
+
+    Ok(())
 }

Consider adding a separate unit test for safe_clamp edge cases (NaN, f32::INFINITY, values near i32::MAX) to validate the conversion safety guarantees. As per coding guidelines: "Test code must not use unwrap() or expect(). Prefer Result-returning tests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/ipc.rs` around lines 1208 - 1235, The test
test_get_device_status_telemetry_conversion must be made into a Result-returning
test (e.g., -> Result<(), Box<dyn std::error::Error>>) and replace .expect()
calls on service.get_device_status() and status.telemetry with the ? operator
and proper pattern matching so no unwrap/expect is used; then add a new unit
test for the safe_clamp function that exercises edge cases (NaN, f32::INFINITY,
f32::NEG_INFINITY, values just above/below i32::MAX/i32::MIN and typical
extremes) validating the returned i32 bounds and that NaN is handled
deterministically, using assert_eq!/assert_ne! on the converted values and
returning Result as well to follow the no-unwrap guideline.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

enable_virtual_devices currently behaves like force_virtual_devices.

Line 63 bypasses create_hid_port() whenever the flag is true. Because WheelService::new() now calls new_with_flags(FeatureFlags::default(), ...) and FeatureFlags::default() in crates/service/src/system_config.rs:220-231 sets enable_virtual_devices: true, the default service path never probes real hardware and boots against the seeded virtual wheel instead. If this flag is meant to allow fallback, try the platform backend first and only create VirtualHidPort on failure; if it is meant to force virtual-only mode, it cannot stay enabled by default.

🐛 Suggested shape

-        let mut real_port_failed = false;
-        let hid_port: Arc<dyn HidPort> = if flags.enable_virtual_devices {
-            info!("Virtual devices explicitly requested via feature flags");
-            Self::create_virtual_port()?
-        } else {
-            match racing_wheel_engine::hid::create_hid_port() {
-                Ok(port) => {
-                    info!("Platform HID backend initialized successfully");
-                    Arc::from(port)
-                }
-                Err(e) => {
-                    warn!(error = %e, "Failed to initialize platform HID backend; falling back to virtual devices");
-                    real_port_failed = true;
-                    Self::create_virtual_port()?
-                }
-            }
-        };
+        let mut real_port_failed = false;
+        let hid_port: Arc<dyn HidPort> = match racing_wheel_engine::hid::create_hid_port() {
+            Ok(port) => {
+                info!("Platform HID backend initialized successfully");
+                Arc::from(port)
+            }
+            Err(e) if flags.enable_virtual_devices => {
+                warn!(error = %e, "Failed to initialize platform HID backend; falling back to virtual devices");
+                real_port_failed = true;
+                Self::create_virtual_port()?
+            }
+            Err(e) => {
+                return Err(anyhow::anyhow!(
+                    "Failed to initialize platform HID backend: {}",
+                    e
+                ));
+            }
+        };

If you still need an explicit virtual-only mode for tests, split that into a separate flag/constructor instead of overloading enable_virtual_devices.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Initialize HID port
	// We attempt to initialize the real platform HID backend first.
	// If it fails or if virtual devices are explicitly requested, we fall back to VirtualHidPort.
	let mut real_port_failed = false;
	let hid_port: Arc<dyn HidPort> = if flags.enable_virtual_devices {
	info!("Virtual devices explicitly requested via feature flags");
	Self::create_virtual_port()?
	} else {
	match racing_wheel_engine::hid::create_hid_port() {
	Ok(port) => {
	info!("Platform HID backend initialized successfully");
	Arc::from(port)
	}
	Err(e) => {
	warn!(error = %e, "Failed to initialize platform HID backend; falling back to virtual devices");
	real_port_failed = true;
	Self::create_virtual_port()?
	}
	// Initialize HID port
	// We attempt to initialize the real platform HID backend first.
	// If it fails or if virtual devices are explicitly requested, we fall back to VirtualHidPort.
	let mut real_port_failed = false;
	let hid_port: Arc<dyn HidPort> = match racing_wheel_engine::hid::create_hid_port() {
	Ok(port) => {
	info!("Platform HID backend initialized successfully");
	Arc::from(port)
	}
	Err(e) if flags.enable_virtual_devices => {
	warn!(error = %e, "Failed to initialize platform HID backend; falling back to virtual devices");
	real_port_failed = true;
	Self::create_virtual_port()?
	}
	Err(e) => {
	return Err(anyhow::anyhow!(
	"Failed to initialize platform HID backend: {}",
	e
	));
	}
	};

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/service/src/service.rs` around lines 59 - 76, The current logic treats
enable_virtual_devices as "force virtual", bypassing
racing_wheel_engine::hid::create_hid_port() whenever it's true; change it so the
code always attempts create_hid_port() first and only calls
Self::create_virtual_port() if create_hid_port() returns Err (or, alternatively,
add a separate force_virtual_devices flag and only skip create_hid_port() when
that new flag is set); update the initialization in
WheelService::new()/new_with_flags and FeatureFlags::default() accordingly so
the default feature flags do not force virtual devices, and keep references to
create_hid_port(), create_virtual_port(), enable_virtual_devices,
WheelService::new()/new_with_flags, and FeatureFlags::default() to locate the
change.

[github-actions]

Compatibility Layer Usage Report

Current usage count: 194
Baseline usage count: 194
ℹ️ Usage unchanged

📈 Usage Trend (Last 30 Days)

• Peak usage: 194
• Current trend: insufficient_data (0%)
• Projected removal: unknown

Usage Details

• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:189 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:192 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:211 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/ipc_conversion.rs:608 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/integration_test.rs:79 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:188 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/schemas/src/service_example.rs:217 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/ui/src/commands.rs:357 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:285 - temp_c
• /home/runner/work/OpenRacing/OpenRacing/crates/engine/src/protocol.rs:737 - temp_c

... and 184 more occurrences

Migration Guide

To migrate these usages, replace:

• .temp_c() → .temperature_c
• .faults() → .fault_flags
• .wheel_angle_mdeg() → .wheel_angle_deg
• .wheel_speed_mrad_s() → .wheel_speed_rad_s

See Migration Patterns for detailed examples.

[coderabbitai]

♻️ Duplicate comments (1)

crates/openracing-native-plugin/src/spsc.rs (1)

342-357: ⚠️ Potential issue | 🟠 Major

Tests still use expect(...); convert both tests to Result-returning style.

These two updated tests still violate the test error-handling rule and should avoid expect(...) (and panic-branch matching can be replaced with matches! assertions).

Proposed fix

 #[test]
-fn test_spsc_frame_size_mismatch() {
+fn test_spsc_frame_size_mismatch() -> Result<(), NativePluginError> {
     let frame_size = 64;
-    let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
+    let channel = SpscChannel::new(frame_size)?;
     let writer = channel.writer();

     let oversized_frame = vec![0x42u8; frame_size + 1];
-    let err = writer.write(&oversized_frame);
-    assert!(err.is_err(), "Expected error for oversized frame");
-    match err {
-        Err(NativePluginError::FrameSizeMismatch { expected, actual }) => {
-            assert_eq!(expected, 64);
-            assert_eq!(actual, 65);
-        }
-        other => panic!("Expected FrameSizeMismatch, got {other:?}"),
-    }
+    let result = writer.write(&oversized_frame);
+    assert!(matches!(
+        result,
+        Err(NativePluginError::FrameSizeMismatch { expected: 64, actual: 65 })
+    ));
+    Ok(())
 }

 #[test]
-fn test_spsc_buffer_size_mismatch() {
+fn test_spsc_buffer_size_mismatch() -> Result<(), NativePluginError> {
     let frame_size = 64;
-    let channel = SpscChannel::new(frame_size).expect("Failed to create channel");
+    let channel = SpscChannel::new(frame_size)?;

     let writer = channel.writer();
-    writer
-        .write(&vec![0x42u8; frame_size])
-        .expect("Failed to write");
+    writer.write(&vec![0x42u8; frame_size])?;

     let reader = channel.reader();
     let mut undersized_buffer = vec![0u8; frame_size - 1];
-    let err = reader.read(&mut undersized_buffer);
-    assert!(err.is_err(), "Expected error for undersized buffer");
-    match err {
-        Err(NativePluginError::BufferSizeMismatch { expected, actual }) => {
-            assert_eq!(expected, 64);
-            assert_eq!(actual, 63);
-        }
-        other => panic!("Expected BufferSizeMismatch, got {other:?}"),
-    }
+    let result = reader.read(&mut undersized_buffer);
+    assert!(matches!(
+        result,
+        Err(NativePluginError::BufferSizeMismatch { expected: 64, actual: 63 })
+    ));
+    Ok(())
 }

As per coding guidelines, "Avoid unwrap() and expect() in test code; use Result-returning tests or explicit assertions instead."

Also applies to: 360-380

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/openracing-native-plugin/src/spsc.rs` around lines 342 - 357, Change
the test function test_spsc_frame_size_mismatch to return Result<(), Box<dyn
std::error::Error>> instead of panicking: replace
SpscChannel::new(...).expect(...) with let channel =
SpscChannel::new(frame_size)? so failures propagate as errors, and replace the
current panic-style match on writer.write(...) with an assertion using matches!
to assert Err(NativePluginError::FrameSizeMismatch{..}) and then assert_eq! on
the extracted expected/actual values (or pattern-match after confirming the Err
variant) instead of panicking; apply the same conversion (remove expect and
return Result) to the other test referenced around lines 360-380 that uses
expect/panic matching.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/openracing-native-plugin/src/spsc.rs`:
- Around line 342-357: Change the test function test_spsc_frame_size_mismatch to
return Result<(), Box<dyn std::error::Error>> instead of panicking: replace
SpscChannel::new(...).expect(...) with let channel =
SpscChannel::new(frame_size)? so failures propagate as errors, and replace the
current panic-style match on writer.write(...) with an assertion using matches!
to assert Err(NativePluginError::FrameSizeMismatch{..}) and then assert_eq! on
the extracted expected/actual values (or pattern-match after confirming the Err
variant) instead of panicking; apply the same conversion (remove expect and
return Result) to the other test referenced around lines 360-380 that uses
expect/panic matching.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b78806a9-31dc-4c2d-8e55-462d18e02ffc

📥 Commits

Reviewing files that changed from the base of the PR and between d3f161c and e28e172.

📒 Files selected for processing (4)

• crates/openracing-native-plugin/src/spsc.rs
• crates/service/src/daemon.rs
• crates/service/src/ipc.rs
• crates/service/src/service.rs