[rghamilton3]

Summary

Implements the ProviderHost runtime infrastructure for managing sensor providers, including:

• ProviderHost - Lifecycle management for providers with async polling, graceful shutdown, and panic containment
• MockProvider - Test provider for development and testing
• LinuxProvider - Real Linux system metrics via /proc and /sys
• Tauri commands - get_providers_status and list_available_sensors for frontend integration
• Comprehensive tests - Unit, integration, concurrency, and property-based tests

Key Features

• Panic containment: Panicking providers update health status without crashing the runtime
• Defensive registration: Unknown sensors encountered during polling trigger re-discovery
• Accurate sensor counts: Each provider tracks its registered sensors
• Proper error handling: RwLock failures return errors instead of panicking

Code Review Fixes

This commit also addresses code review findings:

• High: Added missing host.start() call to enable provider polling
• Medium: Fixed sensor_count to track actual registered sensors per provider
• Low: Improved RwLock error handling in Tauri commands

Test Plan

• All 174 workspace tests pass
• cargo check --workspace clean
• cargo clippy clean
• cargo fmt clean

[github-actions]

Now I have a complete picture of the PR. Let me compile my review findings:

---

Code Review Summary

High Priority Issues

1. RwLock unwrap() in get_providers_status() (host.rs:362)

let health = entry.health.read().unwrap().clone();

This can panic on lock poisoning. The PR description claims "Improved RwLock error handling" but this method still panics. Should return a default or use map_err:

let health = entry.health.read()
    .map(|g| g.clone())
    .unwrap_or(ProviderHealth::Error { 
        message: "Lock poisoned".to_string() 
    });

2. Sensor ID collision in find_temp_sensors() (linux.rs:266-276)

Multiple hwmon devices matching coretemp|cpu|k10temp|k8temp all map to cpu.temperature. Only the last discovered sensor survives. Should include the index:

let sensor_id = if device_name.contains("coretemp") || ... {
    format!("cpu.temp{}.temperature", index)  // or cpu.core{index}.temperature
}

Medium Priority Issues

3. Unused min_poll_interval_ms config (config.rs:5)

The config field exists but is never used to clamp provider poll intervals. Either implement clamping or remove the field.

4. Unused thiserror dependency (provider-host/Cargo.toml:13)

thiserror is in dependencies but ProviderError manually implements Display/Error. Either use #[derive(thiserror::Error)] or remove the dependency.

5. Test code duplication

PanickingProvider, PollPanickingProvider, and HealthyProvider are duplicated in both host.rs (lines 415-543) and host_tests.rs. Consider extracting to a shared test utility module.

Low Priority Issues

6. Inconsistent RwLock error handling

• lib.rs:22 - uses map_err (good)
• host.rs:362-363 - uses unwrap() (bad)
• host.rs:255 - uses if let Ok(...) (good)

Pick one pattern consistently.

7. list_available_sensors provider_id derivation (lib.rs:44-47)

Deriving provider_id from sensor ID prefix (first component before .) may not match the actual provider. Consider storing the provider_id during registration.

Positive Observations

• Excellent panic containment with catch_unwind
• Good defensive registration for unknown sensors
• Clean separation of concerns between crates
• Comprehensive test coverage including ignored tests for platform-specific code
• Sensor ID naming follows conventions from docs/specs/sensor-schema.md
• The #[non_exhaustive] on HostConfig is good API design

Suggested Fix for Issue #1

pub fn get_providers_status(&self) -> Vec<ProviderStatus> {
    self.providers.iter().map(|(id, entry)| {
        let health = entry.health.read()
            .map(|g| g.clone())
            .unwrap_or(ProviderHealth::Error { 
                message: "Lock poisoned".to_string() 
            });
        let sensor_count = entry.sensor_ids.read()
            .map(|ids| ids.len())
            .unwrap_or(0);
        ProviderStatus {
            id: id.clone(),
            name: entry.provider.manifest().name.clone(),
            health,
            sensor_count,
        }
    }).collect()
}

Would you like me to implement any of these fixes?

opencode session  |  github run

[github-actions]

Based on my review of the code, I can confirm the existing review findings are accurate. Here's my additional analysis:

Confirmed Issues

1. RwLock unwrap in get_providers_status() (host.rs:362) ✅ Confirmed
Line 362 still panics on lock poisoning while line 363 correctly uses map().unwrap_or(0). Inconsistent handling.

2. Sensor ID collision (linux.rs:266-276) ✅ Confirmed
Multiple CPU temperature sensors (coretemp, k10temp, etc.) all map to cpu.temperature, losing data from all but the last discovered sensor.

3. Unused min_poll_interval_ms ✅ Confirmed (config.rs:5)
Field exists but is never used to clamp provider poll intervals.

4. Unused thiserror dependency ✅ Confirmed (error.rs)
Manual Display/Error implementations when thiserror is in Cargo.toml.

5. Test code duplication ✅ Confirmed
PanickingProvider, PollPanickingProvider, make_manifest duplicated in both host.rs:394-543 and host_tests.rs.

Additional Finding

8. Provider ID derivation issue (lib.rs:43-47) - More problematic than noted:

let provider_id = sensor_id.split('.').next().unwrap_or(&sensor_id).to_string();

For sensor cpu.temperature, this returns "cpu" which doesn't match the actual provider "linux.provider". The MockProvider produces mock.sensor → provider_id "mock", not "mock.provider".

Recommendation

All High and Medium priority issues should be addressed before merge. The sensor ID collision (Issue #2) is particularly concerning for production Linux systems with multiple CPU cores or thermal zones.

opencode session  |  github run

[github-actions]

Author identity unknown

*** Please tell me who you are.

Run

git config --global user.email "you@example.com"
git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for runner@runnervm0kj6c.mp5piie4oadufatu131nbwrohf.phxx.internal.cloudapp.net) not allowed

opencode session  |  github run