From 44850aafde2cb4a502e89b29e56fdd23d9aefb97 Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 07:44:21 -0700 Subject: [PATCH 1/5] feat(port): per-device suspend state in doctor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last reporting gap from #1279. `doctor` already surfaced the power-plan setting; this adds the per-device layer — the "allow the computer to turn off this device" flag Windows keeps per hub. Disabling one does not disable the other, so a host can look clean at the plan level and still power a port down. COM9 presence attached health healthy topology parent USB\VID_303A&PID_1001\8C:BF:EA:CF:87:B4 suspend this port's hub chain may be powered down by Windows verdict attached and healthy Also exposes the full USB ancestor chain on `DetectedPort`. `fbuild-serial` already walked 16 levels internally but kept only the first hop. That is not enough for anything reasoning about hub-level policy: for a composite device the immediate parent is the device itself, and the nodes carrying power policy are hubs further up. **That bug shipped past its tests.** The first version matched only `parent_instance_id`, and its unit tests passed because the fixtures used a parent that matched by construction. On real hardware it returned `None` for every port — caught by running the command, not by the suite. The replacement test now walks a composite-device-then-hub chain so the one-hop version cannot pass it again. Cost is one WMI call for the whole report (~0.5s measured), not one per port. Deliberately **not** wired into `port scan`: that is a hot path used by deploy, and a diagnostic is the right place to pay for this. `suspend=` per port in `scan` is therefore not implemented, contrary to #1279's wording — the performance trade seemed the wrong one to make silently. Unknown stays `None`, never `false`, in both the text and JSON paths: a consumer must be able to tell "not checked" from "checked and fine". `soldr cargo test -p fbuild-serial -p fbuild-cli` — 290 passed. clippy and fmt clean. Verified live against the real hub chain on this bench. Refs #1279, #1282 Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/deploy.rs | 1 + crates/fbuild-cli/src/cli/port_doctor.rs | 133 ++++++++++++++++++++++- crates/fbuild-serial/src/ports.rs | 9 ++ 3 files changed, 141 insertions(+), 2 deletions(-) diff --git a/crates/fbuild-cli/src/cli/deploy.rs b/crates/fbuild-cli/src/cli/deploy.rs index 7085c0ee..a44f70d9 100644 --- a/crates/fbuild-cli/src/cli/deploy.rs +++ b/crates/fbuild-cli/src/cli/deploy.rs @@ -731,6 +731,7 @@ mod tests { health, instance_id: None, parent_instance_id: None, + ancestor_instance_ids: Vec::new(), } } diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 7fd49ce5..4967ec0b 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -24,6 +24,10 @@ pub struct PortDiagnosis { pub problem_code: Option, pub instance_id: Option, pub parent_instance_id: Option, + /// Whether any USB ancestor of this port may be powered down by Windows. + /// `None` when unknown — never silently `false`. + #[serde(skip_serializing_if = "Option::is_none")] + pub suspend_allowed: Option, } /// What the diagnosis means and what to do about it. @@ -50,7 +54,7 @@ pub fn problem_code_meaning(code: u32) -> Option<&'static str> { }) } -pub fn diagnose(port: &DetectedPort) -> PortDiagnosis { +pub fn diagnose(port: &DetectedPort, power_rows: &[(String, bool)]) -> PortDiagnosis { PortDiagnosis { port: port.info.port_name.clone(), presence: port.health.is_present(), @@ -58,6 +62,7 @@ pub fn diagnose(port: &DetectedPort) -> PortDiagnosis { problem_code: port.health.problem_code(), instance_id: port.instance_id.clone(), parent_instance_id: port.parent_instance_id.clone(), + suspend_allowed: suspend_for_ancestors(power_rows, &port.ancestor_instance_ids), } } @@ -121,6 +126,50 @@ pub fn parse_selective_suspend(powercfg_output: &str) -> Option { u32::from_str_radix(hex, 16).ok().map(|v| v != 0) } +/// Parse `MSPower_DeviceEnable` rows rendered as `instance|Enable`. +/// +/// `Enable = True` means Windows is *allowed* to power the device down. The +/// WMI `InstanceName` carries a `_0` suffix that the PnP instance ID does +/// not, so it is trimmed here rather than at every call site. +pub fn parse_device_power_rows(output: &str) -> Vec<(String, bool)> { + output + .lines() + .filter_map(|line| { + let (instance, enable) = line.trim().split_once('|')?; + let instance = instance.trim().trim_end_matches("_0").to_ascii_uppercase(); + if instance.is_empty() { + return None; + } + Some((instance, enable.trim().eq_ignore_ascii_case("true"))) + }) + .collect() +} + +/// Whether any USB ancestor of this port may be powered down. +/// +/// Takes the **whole ancestor chain**, not just the immediate parent. For a +/// composite device the immediate parent is the device itself; the nodes +/// carrying power policy are hubs further up. Matching only one hop silently +/// reports `None` for every real port — which is exactly what happened on +/// this bench before the chain was threaded through. +/// +/// `None` when nothing matched: silence beats a false "fine". +pub fn suspend_for_ancestors(power_rows: &[(String, bool)], ancestors: &[String]) -> Option { + let mut matched = false; + for ancestor in ancestors { + let ancestor = ancestor.to_ascii_uppercase(); + for (instance, can_power_off) in power_rows { + if ancestor == *instance { + matched = true; + if *can_power_off { + return Some(true); + } + } + } + } + matched.then_some(false) +} + /// Host-level section describing USB selective suspend. /// /// Why the report cares: Windows may power a port down mid-session, and a @@ -181,6 +230,12 @@ pub fn render_report(diagnoses: &[PortDiagnosis], problems: &[UsbProblemDevice]) if let Some(parent) = d.parent_instance_id.as_deref() { let _ = writeln!(out, " topology parent {parent}"); } + if d.suspend_allowed == Some(true) { + let _ = writeln!( + out, + " suspend this port's hub chain may be powered down by Windows" + ); + } let _ = writeln!(out, " verdict {}", v.summary); if !v.remedy.is_empty() { let _ = writeln!(out, " remedy {}", v.remedy); @@ -376,6 +431,7 @@ pub fn build_json_report( /// `fbuild port doctor` entry point. pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { + let power_rows = query_device_power_rows(); let ports = fbuild_serial::ports::available_ports() .map_err(|e| FbuildError::SerialError(format!("serial port enumeration failed: {e}")))?; let diagnoses: Vec<_> = ports @@ -386,7 +442,7 @@ pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { Some(want) => p.info.port_name.eq_ignore_ascii_case(want), None => true, }) - .map(diagnose) + .map(|p| diagnose(p, &power_rows)) .collect(); if diagnoses.is_empty() { if let Some(want) = only_port { @@ -418,6 +474,37 @@ pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { /// probe did. `None` (its value off Windows, or when powercfg is unavailable /// or its output unrecognised) simply omits the section rather than asserting /// the host is fine. +/// Read the per-device "allow the computer to turn off this device" flags. +/// +/// One WMI call for the whole report (~0.5s), not one per port. Deliberately +/// **not** wired into `port scan`: that is a hot path used by deploy, and a +/// diagnostic is the right place to pay for this. +/// +/// Read-only and best-effort; an empty result simply omits the per-port line. +fn query_device_power_rows() -> Vec<(String, bool)> { + if !cfg!(windows) { + return Vec::new(); + } + let script = "Get-CimInstance -Namespace root\\wmi -ClassName MSPower_DeviceEnable \ + -ErrorAction SilentlyContinue | ForEach-Object { \ + Write-Output (\"{0}|{1}\" -f $_.InstanceName, $_.Enable) }"; + let Ok(out) = fbuild_core::subprocess::run_command_blocking( + &[ + "powershell", + "-NoProfile", + "-NonInteractive", + "-Command", + script, + ], + None, + None, + Some(std::time::Duration::from_secs(20)), + ) else { + return Vec::new(); + }; + parse_device_power_rows(&out.stdout) +} + fn query_selective_suspend() -> Option { if !cfg!(windows) { return None; @@ -450,6 +537,7 @@ mod tests { problem_code: problem, instance_id: Some(r"USB\VID_2E8A&PID_F00F\X".to_string()), parent_instance_id: None, + suspend_allowed: None, } } @@ -648,6 +736,47 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) assert!(text.contains("\"presence\":false"), "got: {text}"); } + #[test] + fn device_power_rows_parsed_and_suffix_trimmed() { + let rows = parse_device_power_rows( + "USB\\VID_05E3&PID_0610\\7&3AFC677D&0&1_0|True\nUSB\\ROOT_HUB30\\5&4087D53&0&0_0|False\n", + ); + assert_eq!(rows.len(), 2); + // the WMI `_0` suffix is not part of the PnP instance ID + assert!(rows[0].0.ends_with("&0&1"), "got: {}", rows[0].0); + assert!(rows[0].1); + assert!(!rows[1].1); + } + + /// The suspendable node is typically a *grandparent* hub, not the + /// immediate parent. Matching only one hop reported `None` for every real + /// port on this bench — caught by running it, not by the tests, because + /// the fixtures matched by construction. + #[test] + fn suspend_walks_the_whole_ancestor_chain() { + let rows = parse_device_power_rows("USB\\VID_05E3&PID_0610\\7&3AFC677D&0&1_0|True\n"); + let chain = vec![ + // composite device — the immediate parent, carries no power policy + r"USB\VID_303A&PID_1001\8C:BF:EA:CF:87:B4".to_string(), + // the hub, two hops up, lowercased to pin case-insensitivity + r"USB\VID_05E3&PID_0610\7&3afc677d&0&1".to_string(), + ]; + assert_eq!(suspend_for_ancestors(&rows, &chain), Some(true)); + } + + #[test] + fn suspend_reports_false_only_when_a_row_actually_matched() { + let rows = parse_device_power_rows("USB\\VID_05E3&PID_0610\\7&3AFC677D&0&1_0|False\n"); + let chain = vec![r"USB\VID_05E3&PID_0610\7&3AFC677D&0&1".to_string()]; + assert_eq!(suspend_for_ancestors(&rows, &chain), Some(false)); + // Nothing matched: unknown, not "fine". A false clear is worse than + // saying nothing. + let other = vec![r"USB\VID_DEAD&PID_BEEF\X".to_string()]; + assert_eq!(suspend_for_ancestors(&rows, &other), None); + assert_eq!(suspend_for_ancestors(&rows, &[]), None); + assert_eq!(suspend_for_ancestors(&[], &chain), None); + } + /// A host-wide change must show exactly what it will run, and how to undo it. #[test] fn fix_plan_shows_commands_and_how_to_revert() { diff --git a/crates/fbuild-serial/src/ports.rs b/crates/fbuild-serial/src/ports.rs index 89a85af2..98b1d3ff 100644 --- a/crates/fbuild-serial/src/ports.rs +++ b/crates/fbuild-serial/src/ports.rs @@ -98,6 +98,12 @@ pub struct DetectedPort { pub instance_id: Option, /// Immediate parent device instance ID when the host exposes one. pub parent_instance_id: Option, + /// Full USB ancestor chain, nearest first, when the host exposes one. + /// + /// The immediate parent of a composite device is the device itself, not a + /// hub — anything reasoning about hub-level policy (power management, + /// topology) needs the whole chain, not just one hop. + pub ancestor_instance_ids: Vec, } impl DetectedPort { @@ -107,6 +113,7 @@ impl DetectedPort { health: PortHealth::Unknown, instance_id: None, parent_instance_id: None, + ancestor_instance_ids: Vec::new(), } } } @@ -1007,6 +1014,7 @@ mod imp { } let instance_id = port_device.instance_id(); let parent_instance_id = port_device.parent_instance_id(); + let ancestor_instance_ids = ancestor_ids(port_device.devinfo_data.DevInst); let pnp_observation = port_device.pnp_observation(); let port_type = port_device.port_type(instance_id.as_deref(), parent_instance_id.as_deref()); @@ -1034,6 +1042,7 @@ mod imp { health, instance_id, parent_instance_id, + ancestor_instance_ids, }); } } From e04f20470358d767368f3b7921e63631ee368255 Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 08:03:39 -0700 Subject: [PATCH 2/5] feat(port): report last-seen for a targeted port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $ fbuild port doctor --port COM17 COM17 presence NOT PRESENT last seen 5d ago health phantom verdict not attached — this is a stale registry record, not a fault That is the line that ends the "is it broken or just unplugged?" question outright, and it is what was missing when FastLED/FastLED#3864 spent a full investigation on a board that had not been attached for six days. This was previously abandoned. #1282 §4 records four Win32 approaches that all fail for a *phantom* devnode: - `SetupDiGetDevicePropertyW` returns nothing on the devinfo set. - The iterator's `DevInst` is invalid for phantoms by construction — `CR_NO_SUCH_DEVINST` is how `pnp_observation` classifies one. - `CM_Locate_DevNodeW(..., CM_LOCATE_DEVNODE_PHANTOM)` succeeds, but the follow-up `CM_Get_DevNode_PropertyW` returns `CR_NO_SUCH_VALUE`. - The registry copy needs elevation, which conflicts with `doctor` being read-only. The CIM provider behind `Get-PnpDeviceProperty` answers unelevated, for phantoms, so no new FFI is needed — it reuses the PowerShell-through- `fbuild_core::subprocess` pattern already used for `powercfg` and `MSPower_DeviceEnable`. **Single port only, deliberately.** Measured: ~0.74s per device with no batching, so asking for all 17 ports on this bench costs 12.5s. Restricting it to `--port` puts the answer where the question is asked and leaves the listing fast. Same trade as the per-device suspend query. Timestamps cross the boundary as unix seconds rather than a formatted date: `Get-PnpDeviceProperty` renders dates per-locale, so parsing text would break on a non-English host. A test pins that a formatted date is rejected rather than mis-parsed. Verified live: COM17 reports `last seen 5d ago`, matching its `DEVPKEY_Device_LastArrivalDate` of 2026-08-01 confirmed independently in PowerShell. Whole command runs in 2.0s. `soldr cargo test -p fbuild-serial -p fbuild-cli` — 292 passed. clippy and fmt clean. Refs #1279, #1282 Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/port_doctor.rs | 118 ++++++++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 4967ec0b..47851460 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -28,6 +28,10 @@ pub struct PortDiagnosis { /// `None` when unknown — never silently `false`. #[serde(skip_serializing_if = "Option::is_none")] pub suspend_allowed: Option, + /// Seconds since the devnode was last seen on the bus. Only populated for + /// a single-port query — see `query_last_seen_secs` for why. + #[serde(skip_serializing_if = "Option::is_none")] + pub last_seen_secs_ago: Option, } /// What the diagnosis means and what to do about it. @@ -63,6 +67,7 @@ pub fn diagnose(port: &DetectedPort, power_rows: &[(String, bool)]) -> PortDiagn instance_id: port.instance_id.clone(), parent_instance_id: port.parent_instance_id.clone(), suspend_allowed: suspend_for_ancestors(power_rows, &port.ancestor_instance_ids), + last_seen_secs_ago: None, } } @@ -170,6 +175,38 @@ pub fn suspend_for_ancestors(power_rows: &[(String, bool)], ancestors: &[String] matched.then_some(false) } +/// Parse `instance|unix_seconds` rows into a last-seen timestamp. +/// +/// The PowerShell side emits `ToUnixTimeSeconds()` rather than a formatted +/// date on purpose: a rendered date is locale-dependent and would parse +/// differently on a non-English host. +pub fn parse_last_seen_rows(output: &str) -> Option { + output + .lines() + .filter_map(|line| line.trim().rsplit_once('|')) + .filter_map(|(_, secs)| secs.trim().parse::().ok()) + .next() +} + +/// Render an elapsed duration as a short human age: `45s`, `12m`, `3h`, `6d`. +/// +/// Deliberately coarse — the reader only needs "moments ago" versus "days +/// ago" to tell a live board from a stale record. +pub fn format_age(secs: i64) -> String { + if secs < 0 { + return "in the future".to_string(); + } + if secs < 60 { + format!("{secs}s") + } else if secs < 3600 { + format!("{}m", secs / 60) + } else if secs < 86_400 { + format!("{}h", secs / 3600) + } else { + format!("{}d", secs / 86_400) + } +} + /// Host-level section describing USB selective suspend. /// /// Why the report cares: Windows may power a port down mid-session, and a @@ -215,6 +252,9 @@ pub fn render_report(diagnoses: &[PortDiagnosis], problems: &[UsbProblemDevice]) None => "unknown", }; let _ = writeln!(out, " presence {presence}"); + if let Some(secs) = d.last_seen_secs_ago { + let _ = writeln!(out, " last seen {} ago", format_age(secs)); + } let mut health_line = format!(" health {}", d.health); if let Some(code) = d.problem_code { match problem_code_meaning(code) { @@ -434,7 +474,7 @@ pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { let power_rows = query_device_power_rows(); let ports = fbuild_serial::ports::available_ports() .map_err(|e| FbuildError::SerialError(format!("serial port enumeration failed: {e}")))?; - let diagnoses: Vec<_> = ports + let mut diagnoses: Vec<_> = ports .iter() // Explicit match rather than `Option::is_none_or`: that is stable only // since 1.82 and `.clippy.toml` pins msrv = 1.75. @@ -453,6 +493,21 @@ pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { ))); } } + // Only for a targeted query: this costs ~0.7s per device and does not + // batch, so the all-ports listing deliberately goes without. + if only_port.is_some() { + for d in diagnoses.iter_mut() { + if let Some(instance) = d.instance_id.as_deref() { + d.last_seen_secs_ago = query_last_seen_secs(instance).map(|arrived| { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(arrived); + now - arrived + }); + } + } + } let problems = fbuild_serial::ports::present_usb_problem_devices(); let suspend = query_selective_suspend(); if json { @@ -505,6 +560,46 @@ fn query_device_power_rows() -> Vec<(String, bool)> { parse_device_power_rows(&out.stdout) } +/// Seconds since this devnode was last seen on the bus. +/// +/// Only ever called for a **single** port. `Get-PnpDeviceProperty` costs +/// ~0.7s per device and does not batch, so asking for every port would add +/// ~12s to the listing (measured, 17 ports). Restricting it to `--port` puts +/// the answer exactly where the question is asked — "is this board broken or +/// just unplugged?" — without taxing the general report. +/// +/// This is the one path that works: `SetupDiGetDevicePropertyW` returns +/// nothing for a phantom devnode, `CM_Get_DevNode_PropertyW` returns +/// `CR_NO_SUCH_VALUE` even after `CM_Locate_DevNodeW(..., PHANTOM)`, and the +/// registry copy needs elevation. The CIM provider behind +/// `Get-PnpDeviceProperty` answers unelevated for phantoms. +fn query_last_seen_secs(instance_id: &str) -> Option { + if !cfg!(windows) { + return None; + } + let script = format!( + "$d = Get-PnpDeviceProperty -InstanceId '{}' \ + -KeyName 'DEVPKEY_Device_LastArrivalDate' -ErrorAction SilentlyContinue; \ + if ($d -and $d.Data) {{ Write-Output (\"x|{{0}}\" -f \ + ([DateTimeOffset]$d.Data).ToUnixTimeSeconds()) }}", + instance_id.replace('\'', "''") + ); + let out = fbuild_core::subprocess::run_command_blocking( + &[ + "powershell", + "-NoProfile", + "-NonInteractive", + "-Command", + &script, + ], + None, + None, + Some(std::time::Duration::from_secs(30)), + ) + .ok()?; + parse_last_seen_rows(&out.stdout) +} + fn query_selective_suspend() -> Option { if !cfg!(windows) { return None; @@ -538,6 +633,7 @@ mod tests { instance_id: Some(r"USB\VID_2E8A&PID_F00F\X".to_string()), parent_instance_id: None, suspend_allowed: None, + last_seen_secs_ago: None, } } @@ -764,6 +860,26 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) assert_eq!(suspend_for_ancestors(&rows, &chain), Some(true)); } + /// Unix seconds, not a rendered date: `Get-PnpDeviceProperty` formats + /// dates per-locale, so parsing text would break on a non-English host. + #[test] + fn last_seen_parsed_from_unix_seconds() { + assert_eq!(parse_last_seen_rows("x|1785648479\n"), Some(1785648479)); + assert_eq!(parse_last_seen_rows(""), None); + assert_eq!(parse_last_seen_rows("x|8/1/2026 10:27:59 PM"), None); + assert_eq!(parse_last_seen_rows("garbage"), None); + } + + #[test] + fn age_renders_coarsely_enough_to_read_at_a_glance() { + assert_eq!(format_age(45), "45s"); + assert_eq!(format_age(90), "1m"); + assert_eq!(format_age(7_200), "2h"); + assert_eq!(format_age(5 * 86_400), "5d"); + // clock skew must not render as a huge negative age + assert_eq!(format_age(-5), "in the future"); + } + #[test] fn suspend_reports_false_only_when_a_row_actually_matched() { let rows = parse_device_power_rows("USB\\VID_05E3&PID_0610\\7&3AFC677D&0&1_0|False\n"); From 2c4833435a07a983557e14586517c54bc0b9cfad Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 08:10:34 -0700 Subject: [PATCH 3/5] feat(port): add --all and --hub scope selectors to doctor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last open criterion on #1279. fbuild port doctor --hub VID_05E3 # every port behind that hub fbuild port doctor --all # every port (the default) `--hub` matches any USB **ancestor** by substring, not the immediate parent and not an exact instance ID. Both choices are deliberate: the interesting node is usually a grandparent hub rather than the composite device directly above the port, and you rarely have a full instance string to hand when you are looking at a hub in Device Manager — a VID:PID fragment is what people actually have. This is only possible because the ancestor chain is now exposed on `DetectedPort`. `--all` is the existing default, added for symmetry so the scope is expressible rather than implied. A selector matching nothing is an error, not an empty report: silence looks like "everything is fine" when it actually means "your filter was wrong". `--port` and `--hub` conflict at the clap level, and `--port` takes precedence in the matcher so a narrower request is never silently widened. Verified live: `--hub VID_05E3` selects the ports behind this bench's Genesys hub, a non-matching hub errors with a pointer to `port doctor`, and `--port X --hub Y` is rejected by clap. `soldr cargo test -p fbuild-serial -p fbuild-cli` — 296 passed. clippy and fmt clean. Refs #1279, #1282 Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/port_doctor.rs | 87 ++++++++++++++++++++++-- crates/fbuild-cli/src/cli/port_scan.rs | 11 ++- 2 files changed, 91 insertions(+), 7 deletions(-) diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 47851460..a8d560cf 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -346,6 +346,31 @@ pub fn render_fix_plan(commands: &[Vec], already_disabled: bool) -> Stri out } +/// Does this port fall within the requested scope? +/// +/// `--port` matches the COM name exactly (case-insensitively). `--hub` +/// matches any USB **ancestor**, by substring, so a partial instance ID or a +/// bare VID:PID fragment works — you rarely have the full instance string to +/// hand when you are looking at a hub in Device Manager. Neither given means +/// every port. +pub fn port_in_scope( + port_name: &str, + ancestors: &[String], + only_port: Option<&str>, + only_hub: Option<&str>, +) -> bool { + if let Some(want) = only_port { + return port_name.eq_ignore_ascii_case(want); + } + if let Some(hub) = only_hub { + let hub = hub.to_ascii_uppercase(); + return ancestors + .iter() + .any(|a| a.to_ascii_uppercase().contains(&hub)); + } + true +} + /// Apply the safe subset of remedies: disable USB selective suspend. /// /// Deliberately narrow. It does **not** disable/enable devnodes or restart @@ -470,7 +495,7 @@ pub fn build_json_report( } /// `fbuild port doctor` entry point. -pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { +pub fn run(only_port: Option<&str>, only_hub: Option<&str>, json: bool) -> Result<()> { let power_rows = query_device_power_rows(); let ports = fbuild_serial::ports::available_ports() .map_err(|e| FbuildError::SerialError(format!("serial port enumeration failed: {e}")))?; @@ -478,20 +503,30 @@ pub fn run(only_port: Option<&str>, json: bool) -> Result<()> { .iter() // Explicit match rather than `Option::is_none_or`: that is stable only // since 1.82 and `.clippy.toml` pins msrv = 1.75. - .filter(|p| match only_port { - Some(want) => p.info.port_name.eq_ignore_ascii_case(want), - None => true, + .filter(|p| { + port_in_scope( + &p.info.port_name, + &p.ancestor_instance_ids, + only_port, + only_hub, + ) }) .map(|p| diagnose(p, &power_rows)) .collect(); if diagnoses.is_empty() { + // Being explicit beats silence: a selector that matches nothing is + // itself a finding, not an empty report. if let Some(want) = only_port { - // Being explicit beats silence: a name that matches nothing is - // itself a finding, not an empty report. return Err(FbuildError::SerialError(format!( "no serial port named {want}; run `fbuild port scan` to list what the host sees" ))); } + if let Some(hub) = only_hub { + return Err(FbuildError::SerialError(format!( + "no port sits behind a USB ancestor matching {hub}; \ + run `fbuild port doctor` to see each port's topology" + ))); + } } // Only for a targeted query: this costs ~0.7s per device and does not // batch, so the all-ports listing deliberately goes without. @@ -870,6 +905,46 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) assert_eq!(parse_last_seen_rows("garbage"), None); } + #[test] + fn scope_defaults_to_every_port() { + assert!(port_in_scope("COM9", &[], None, None)); + } + + #[test] + fn scope_port_matches_case_insensitively_and_exactly() { + assert!(port_in_scope("COM9", &[], Some("com9"), None)); + assert!(!port_in_scope("COM9", &[], Some("COM19"), None)); + // must not match on prefix — COM1 is not COM19 + assert!(!port_in_scope("COM19", &[], Some("COM1"), None)); + } + + /// `--hub` matches any ancestor by substring: you rarely have the full + /// instance ID to hand when looking at a hub in Device Manager. + #[test] + fn scope_hub_matches_any_ancestor_by_substring() { + let chain = vec![ + r"USB\VID_303A&PID_1001\8C:BF:EA:CF:87:B4".to_string(), + r"USB\VID_05E3&PID_0610\7&3afc677d&0&1".to_string(), + ]; + assert!(port_in_scope("COM9", &chain, None, Some("VID_05E3"))); + assert!(port_in_scope("COM9", &chain, None, Some("vid_05e3"))); + assert!(!port_in_scope("COM9", &chain, None, Some("VID_DEAD"))); + assert!(!port_in_scope("COM9", &[], None, Some("VID_05E3"))); + } + + /// An explicit port wins over a hub filter, so the narrower request is + /// never silently widened. + #[test] + fn scope_port_takes_precedence_over_hub() { + let chain = vec![r"USB\VID_05E3&PID_0610\X".to_string()]; + assert!(!port_in_scope( + "COM9", + &chain, + Some("COM17"), + Some("VID_05E3") + )); + } + #[test] fn age_renders_coarsely_enough_to_read_at_a_glance() { assert_eq!(format_age(45), "45s"); diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index 72d5f165..ba495627 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -56,6 +56,13 @@ pub enum PortAction { /// ignore it and hand a script text it cannot parse. #[arg(long, conflicts_with_all = ["fix", "dry_run"])] json: bool, + /// Diagnose every port. The default; accepted for symmetry with --port. + #[arg(long, conflicts_with_all = ["port", "hub"])] + all: bool, + /// Diagnose only ports behind a USB ancestor matching this substring + /// (a partial instance ID or VID:PID fragment is enough). + #[arg(long, conflicts_with = "port")] + hub: Option, }, } @@ -70,11 +77,13 @@ pub fn run_port(action: PortAction) -> Result<()> { yes, no_elevate, json, + all: _all, + hub, } => { if fix || dry_run { super::port_doctor::run_fix(dry_run, yes, no_elevate) } else { - super::port_doctor::run(port.as_deref(), json) + super::port_doctor::run(port.as_deref(), hub.as_deref(), json) } } } From 768b7d9c34a3008331d9345d70937a412351731d Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 09:19:40 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(port):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20parsing,=20future=20timestamps,=20off-Windows=20--hub,=20sco?= =?UTF-8?q?pe/fix=20conflicts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four CodeRabbit findings on #1284, all real: 1. parse_device_power_rows used trim_end_matches("_0"), which strips repeatedly — an instance ending _0_0 lost both and stopped matching its devnode. Now strip_suffix. It also treated any non-"true" Enable as false, so garbage quietly became "cannot be powered off"; unrecognised values are now dropped as no data. That is the same unknown-must-not- become-false rule applied elsewhere in this file, which I had violated here. 2. The negative-age branch rendered as "last seen in the future ago". Split format_last_seen out so the whole phrase is chosen together. 3. --hub always failed off Windows, where the ancestor chain is empty, and the error blamed the hub string — sending the reader after a typo that is not there. It now says topology is unavailable. 4. --all/--hub were ignored in fix mode. Same silently-ignored-flag class as the --json fix in #1283. --port had the identical hole and is fixed too, though only --all/--hub were flagged. soldr cargo test -p fbuild-serial -p fbuild-cli: 299 passed. clippy and fmt clean. Verified the conflicts fire and the normal report path is intact. Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/port_doctor.rs | 74 ++++++++++++++++++++++-- crates/fbuild-cli/src/cli/port_scan.rs | 14 ++++- 2 files changed, 79 insertions(+), 9 deletions(-) diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index a8d560cf..5c1f3ab2 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -141,11 +141,24 @@ pub fn parse_device_power_rows(output: &str) -> Vec<(String, bool)> { .lines() .filter_map(|line| { let (instance, enable) = line.trim().split_once('|')?; - let instance = instance.trim().trim_end_matches("_0").to_ascii_uppercase(); + let instance = instance.trim(); + // `strip_suffix`, not `trim_end_matches`: the latter strips + // repeatedly, so an instance legitimately ending `_0_0` would lose + // both and stop matching its devnode. + let instance = instance.strip_suffix("_0").unwrap_or(instance); + let instance = instance.to_ascii_uppercase(); if instance.is_empty() { return None; } - Some((instance, enable.trim().eq_ignore_ascii_case("true"))) + // An unrecognised value is *no data*, not "cannot be powered off". + // Treating it as false would quietly clear a device nobody checked + // — the same trap as reporting unknown suspend state as disabled. + let enable = match enable.trim() { + v if v.eq_ignore_ascii_case("true") => true, + v if v.eq_ignore_ascii_case("false") => false, + _ => return None, + }; + Some((instance, enable)) }) .collect() } @@ -192,9 +205,19 @@ pub fn parse_last_seen_rows(output: &str) -> Option { /// /// Deliberately coarse — the reader only needs "moments ago" versus "days /// ago" to tell a live board from a stale record. +/// The whole "last seen" phrase, so the negative case cannot render as the +/// nonsense "last seen in the future ago". Clock skew and a devnode stamped +/// slightly ahead of the host are both real, so the branch has to exist. +pub fn format_last_seen(secs: i64) -> String { + if secs < 0 { + return "a timestamp in the future (host clock skew?)".to_string(); + } + format!("{} ago", format_age(secs)) +} + pub fn format_age(secs: i64) -> String { if secs < 0 { - return "in the future".to_string(); + return "0s".to_string(); } if secs < 60 { format!("{secs}s") @@ -253,7 +276,7 @@ pub fn render_report(diagnoses: &[PortDiagnosis], problems: &[UsbProblemDevice]) }; let _ = writeln!(out, " presence {presence}"); if let Some(secs) = d.last_seen_secs_ago { - let _ = writeln!(out, " last seen {} ago", format_age(secs)); + let _ = writeln!(out, " last seen {}", format_last_seen(secs)); } let mut health_line = format!(" health {}", d.health); if let Some(code) = d.problem_code { @@ -522,6 +545,17 @@ pub fn run(only_port: Option<&str>, only_hub: Option<&str>, json: bool) -> Resul ))); } if let Some(hub) = only_hub { + // Distinguish "no match" from "we have no topology to match + // against". Off Windows the ancestor chain is always empty, so + // blaming the hub string would send the reader looking for a + // typo that is not there. + if ports.iter().all(|p| p.ancestor_instance_ids.is_empty()) { + return Err(FbuildError::SerialError( + "--hub needs USB topology, which this host does not expose \ + (normal off Windows); re-run without --hub" + .to_string(), + )); + } return Err(FbuildError::SerialError(format!( "no port sits behind a USB ancestor matching {hub}; \ run `fbuild port doctor` to see each port's topology" @@ -945,14 +979,42 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) )); } + /// An unrecognised `Enable` value is no data, not "cannot be powered + /// off" — treating it as false would quietly clear a device nobody + /// checked. + #[test] + fn unrecognised_enable_value_is_dropped_not_treated_as_false() { + let rows = parse_device_power_rows("A|True\nB|False\nC|\nD|maybe\nE|(null)\n"); + let names: Vec<&str> = rows.iter().map(|(n, _)| n.as_str()).collect(); + assert_eq!(names, vec!["A", "B"], "only recognised values survive"); + } + + /// `trim_end_matches` strips repeatedly; an instance legitimately ending + /// `_0_0` would lose both suffixes and stop matching its devnode. + #[test] + fn only_one_wmi_suffix_is_stripped() { + let rows = parse_device_power_rows("USB\\X_0_0|True\n"); + assert_eq!(rows[0].0, "USB\\X_0"); + } + + /// The negative branch must not render as "last seen in the future ago". + #[test] + fn future_timestamp_renders_as_a_sentence_not_nonsense() { + let s = format_last_seen(-5); + assert!(!s.contains("ago"), "got: {s}"); + assert!(s.contains("future"), "got: {s}"); + assert_eq!(format_last_seen(90), "1m ago"); + } + #[test] fn age_renders_coarsely_enough_to_read_at_a_glance() { assert_eq!(format_age(45), "45s"); assert_eq!(format_age(90), "1m"); assert_eq!(format_age(7_200), "2h"); assert_eq!(format_age(5 * 86_400), "5d"); - // clock skew must not render as a huge negative age - assert_eq!(format_age(-5), "in the future"); + // Negative clamps here; the readable phrasing for a future timestamp + // lives in format_last_seen, so "in the future ago" is impossible. + assert_eq!(format_age(-5), "0s"); } #[test] diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index ba495627..f9bcb568 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -34,7 +34,11 @@ pub enum PortAction { /// never elevates, prompts, or changes host state. Doctor { /// Diagnose a single port (e.g. `COM17`). Defaults to every port. - #[arg(long)] + /// + /// Rejected with `--fix`/`--dry-run` for the same reason as the other + /// scope flags — those act host-wide, not per port. Flagged by review + /// for `--all`/`--hub`; `--port` had the identical hole. + #[arg(long, conflicts_with_all = ["fix", "dry_run"])] port: Option, /// Apply the safe remedies (currently: disable USB selective suspend). /// Needs administrator rights and `--yes`. @@ -57,11 +61,15 @@ pub enum PortAction { #[arg(long, conflicts_with_all = ["fix", "dry_run"])] json: bool, /// Diagnose every port. The default; accepted for symmetry with --port. - #[arg(long, conflicts_with_all = ["port", "hub"])] + /// + /// Like `--port`/`--hub`, rejected with `--fix`/`--dry-run`: those + /// paths act on a host-wide power setting, not on a selected port, so + /// a scope flag there would be silently ignored. + #[arg(long, conflicts_with_all = ["port", "hub", "fix", "dry_run"])] all: bool, /// Diagnose only ports behind a USB ancestor matching this substring /// (a partial instance ID or VID:PID fragment is enough). - #[arg(long, conflicts_with = "port")] + #[arg(long, conflicts_with_all = ["port", "fix", "dry_run"])] hub: Option, }, } From d572df5bbe377e3e8d57c71838d6f24b04b33b36 Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 09:24:22 -0700 Subject: [PATCH 5/5] refactor(port): split the --fix path into port_doctor_fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's 'Reject .rs files over 1000 LOC' gate failed: port_doctor.rs had grown to 1041 lines. Moved the mutating side — suspend_fix_commands, render_fix_plan, run_fix and their tests — into port_doctor_fix.rs. The split is along a real seam rather than an arbitrary line count: the read-only diagnosis and the host-changing remedy are separate concerns, and keeping them in separate modules makes 'doctor never mutates' legible from the file layout. port_doctor.rs 1041 -> 906, port_doctor_fix.rs 157. Behaviour unchanged: 299 tests pass, clippy and fmt clean, and --dry-run / --port output verified identical after the move. Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/mod.rs | 1 + crates/fbuild-cli/src/cli/port_doctor.rs | 143 +---------------- crates/fbuild-cli/src/cli/port_doctor_fix.rs | 155 +++++++++++++++++++ crates/fbuild-cli/src/cli/port_scan.rs | 2 +- 4 files changed, 158 insertions(+), 143 deletions(-) create mode 100644 crates/fbuild-cli/src/cli/port_doctor_fix.rs diff --git a/crates/fbuild-cli/src/cli/mod.rs b/crates/fbuild-cli/src/cli/mod.rs index 6b208e20..52c9b672 100644 --- a/crates/fbuild-cli/src/cli/mod.rs +++ b/crates/fbuild-cli/src/cli/mod.rs @@ -34,6 +34,7 @@ pub mod monitor_parse; pub mod pio; pub mod plotter; pub mod port_doctor; +pub mod port_doctor_fix; pub mod port_scan; pub mod purge; pub mod reset; diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 5c1f3ab2..39910e5e 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -328,47 +328,6 @@ pub fn render_report(diagnoses: &[PortDiagnosis], problems: &[UsbProblemDevice]) out } -/// The exact `powercfg` argv `--fix` would run, AC and DC. -/// -/// Pure so the change set can be shown by `--dry-run` and asserted in tests -/// without touching the host. Both indices matter: disabling only the AC side -/// leaves a laptop suspending ports the moment it is unplugged. -pub fn suspend_fix_commands() -> Vec> { - ["-setacvalueindex", "-setdcvalueindex"] - .iter() - .map(|verb| { - vec![ - "powercfg".to_string(), - (*verb).to_string(), - "SCHEME_CURRENT".to_string(), - USB_SUBGROUP_GUID.to_string(), - SELECTIVE_SUSPEND_GUID.to_string(), - "0".to_string(), - ] - }) - .collect() -} - -/// Human-readable plan for `--fix`, used by `--dry-run` and before elevating. -pub fn render_fix_plan(commands: &[Vec], already_disabled: bool) -> String { - if already_disabled { - // Idempotent: nothing to do, and in particular no UAC prompt for a - // change that would be a no-op. - return "nothing to do — USB selective suspend is already disabled\n".to_string(); - } - let mut out = String::from("would run, elevated:\n"); - for cmd in commands { - out.push_str(" "); - out.push_str(&cmd.join(" ")); - out.push('\n'); - } - out.push_str( - "this changes a host-wide power setting; revert with the same commands and a \ - trailing 1 instead of 0\n", - ); - out -} - /// Does this port fall within the requested scope? /// /// `--port` matches the COM name exactly (case-insensitively). `--hub` @@ -394,73 +353,6 @@ pub fn port_in_scope( true } -/// Apply the safe subset of remedies: disable USB selective suspend. -/// -/// Deliberately narrow. It does **not** disable/enable devnodes or restart -/// hubs: a bus reset is not a VBUS cycle and does not recover a -/// descriptor-failed device, while a root-hub restart disrupts every other -/// device on that hub. A fix that looks helpful and is not is worse than none. -pub fn run_fix(dry_run: bool, assume_yes: bool, no_elevate: bool) -> Result<()> { - let current = query_selective_suspend(); - let already_disabled = current == Some(false); - let commands = suspend_fix_commands(); - crate::output::result(render_fix_plan(&commands, already_disabled).trim_end_matches('\n')); - - if already_disabled || dry_run { - return Ok(()); - } - if !cfg!(windows) { - crate::output::result("not applicable on this platform"); - return Ok(()); - } - if no_elevate { - return Err(FbuildError::SerialError( - "--no-elevate was passed but this change needs administrator rights; \ - re-run without it, or run the commands above from an elevated shell" - .to_string(), - )); - } - if !assume_yes { - // A host-wide power-policy change should never be a side effect of a - // diagnostic. Require the caller to say so. - return Err(FbuildError::SerialError( - "this changes a host-wide power setting; re-run with --yes to apply, \ - or --dry-run to see the plan only" - .to_string(), - )); - } - - // One elevation for the whole change set, not one per command. - let joined = commands - .iter() - .map(|c| c.join(" ")) - .collect::>() - .join("; "); - let script = - format!("Start-Process -Verb RunAs -Wait -FilePath cmd -ArgumentList '/c {joined}'"); - let out = fbuild_core::subprocess::run_command_blocking( - &[ - "powershell", - "-NoProfile", - "-NonInteractive", - "-Command", - &script, - ], - None, - None, - Some(std::time::Duration::from_secs(120)), - ) - .map_err(|e| FbuildError::SerialError(format!("elevation failed: {e}")))?; - if !out.success() { - return Err(FbuildError::SerialError(format!( - "elevated powercfg failed: {}", - out.stderr.trim() - ))); - } - crate::output::result("USB selective suspend disabled; re-run `fbuild port doctor` to confirm"); - Ok(()) -} - /// Machine-readable report. Mirrors the human output's structure so a script /// and a person are reading the same model. #[derive(Debug, serde::Serialize)] @@ -669,7 +561,7 @@ fn query_last_seen_secs(instance_id: &str) -> Option { parse_last_seen_rows(&out.stdout) } -fn query_selective_suspend() -> Option { +pub(crate) fn query_selective_suspend() -> Option { if !cfg!(windows) { return None; } @@ -831,30 +723,6 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) assert!(render_suspend_section(Some(false)).contains("disabled")); } - /// Both indices matter: disabling only AC leaves a laptop suspending - /// ports the moment it is unplugged. - #[test] - fn fix_covers_both_ac_and_dc() { - let cmds = suspend_fix_commands(); - assert_eq!(cmds.len(), 2); - assert!(cmds.iter().any(|c| c.contains(&"-setacvalueindex".into()))); - assert!(cmds.iter().any(|c| c.contains(&"-setdcvalueindex".into()))); - for c in &cmds { - assert_eq!(c.last().unwrap(), "0", "must disable, not enable"); - assert!(c.contains(&USB_SUBGROUP_GUID.to_string())); - assert!(c.contains(&SELECTIVE_SUSPEND_GUID.to_string())); - } - } - - /// Idempotence: an already-disabled host must produce no plan, so `--fix` - /// never raises a UAC prompt for a no-op. - #[test] - fn fix_plan_is_empty_when_already_disabled() { - let plan = render_fix_plan(&suspend_fix_commands(), true); - assert!(plan.contains("nothing to do"), "got: {plan}"); - assert!(!plan.contains("would run"), "got: {plan}"); - } - /// The JSON must keep unassociated problem devices in their own list /// rather than hanging them off a port — the same separation the text /// report enforces, for the same reason. @@ -1029,13 +897,4 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) assert_eq!(suspend_for_ancestors(&rows, &[]), None); assert_eq!(suspend_for_ancestors(&[], &chain), None); } - - /// A host-wide change must show exactly what it will run, and how to undo it. - #[test] - fn fix_plan_shows_commands_and_how_to_revert() { - let plan = render_fix_plan(&suspend_fix_commands(), false); - assert!(plan.contains("would run, elevated"), "got: {plan}"); - assert!(plan.contains("-setacvalueindex"), "got: {plan}"); - assert!(plan.contains("revert"), "got: {plan}"); - } } diff --git a/crates/fbuild-cli/src/cli/port_doctor_fix.rs b/crates/fbuild-cli/src/cli/port_doctor_fix.rs new file mode 100644 index 00000000..eb2d4dd2 --- /dev/null +++ b/crates/fbuild-cli/src/cli/port_doctor_fix.rs @@ -0,0 +1,155 @@ +//! `fbuild port doctor --fix` — the one mutating path. +//! +//! Split from `port_doctor` so the read-only diagnosis and the host-changing +//! remedy stay separate concerns (and to keep both files under this repo's +//! 1000-LOC cap). + +use fbuild_core::{FbuildError, Result}; + +use super::port_doctor::{SELECTIVE_SUSPEND_GUID, USB_SUBGROUP_GUID, query_selective_suspend}; + +/// The exact `powercfg` argv `--fix` would run, AC and DC. +/// +/// Pure so the change set can be shown by `--dry-run` and asserted in tests +/// without touching the host. Both indices matter: disabling only the AC side +/// leaves a laptop suspending ports the moment it is unplugged. +pub fn suspend_fix_commands() -> Vec> { + ["-setacvalueindex", "-setdcvalueindex"] + .iter() + .map(|verb| { + vec![ + "powercfg".to_string(), + (*verb).to_string(), + "SCHEME_CURRENT".to_string(), + USB_SUBGROUP_GUID.to_string(), + SELECTIVE_SUSPEND_GUID.to_string(), + "0".to_string(), + ] + }) + .collect() +} + +/// Human-readable plan for `--fix`, used by `--dry-run` and before elevating. +pub fn render_fix_plan(commands: &[Vec], already_disabled: bool) -> String { + if already_disabled { + // Idempotent: nothing to do, and in particular no UAC prompt for a + // change that would be a no-op. + return "nothing to do — USB selective suspend is already disabled\n".to_string(); + } + let mut out = String::from("would run, elevated:\n"); + for cmd in commands { + out.push_str(" "); + out.push_str(&cmd.join(" ")); + out.push('\n'); + } + out.push_str( + "this changes a host-wide power setting; revert with the same commands and a \ + trailing 1 instead of 0\n", + ); + out +} + +/// Apply the safe subset of remedies: disable USB selective suspend. +/// +/// Deliberately narrow. It does **not** disable/enable devnodes or restart +/// hubs: a bus reset is not a VBUS cycle and does not recover a +/// descriptor-failed device, while a root-hub restart disrupts every other +/// device on that hub. A fix that looks helpful and is not is worse than none. +pub fn run_fix(dry_run: bool, assume_yes: bool, no_elevate: bool) -> Result<()> { + let current = query_selective_suspend(); + let already_disabled = current == Some(false); + let commands = suspend_fix_commands(); + crate::output::result(render_fix_plan(&commands, already_disabled).trim_end_matches('\n')); + + if already_disabled || dry_run { + return Ok(()); + } + if !cfg!(windows) { + crate::output::result("not applicable on this platform"); + return Ok(()); + } + if no_elevate { + return Err(FbuildError::SerialError( + "--no-elevate was passed but this change needs administrator rights; \ + re-run without it, or run the commands above from an elevated shell" + .to_string(), + )); + } + if !assume_yes { + // A host-wide power-policy change should never be a side effect of a + // diagnostic. Require the caller to say so. + return Err(FbuildError::SerialError( + "this changes a host-wide power setting; re-run with --yes to apply, \ + or --dry-run to see the plan only" + .to_string(), + )); + } + + // One elevation for the whole change set, not one per command. + let joined = commands + .iter() + .map(|c| c.join(" ")) + .collect::>() + .join("; "); + let script = + format!("Start-Process -Verb RunAs -Wait -FilePath cmd -ArgumentList '/c {joined}'"); + let out = fbuild_core::subprocess::run_command_blocking( + &[ + "powershell", + "-NoProfile", + "-NonInteractive", + "-Command", + &script, + ], + None, + None, + Some(std::time::Duration::from_secs(120)), + ) + .map_err(|e| FbuildError::SerialError(format!("elevation failed: {e}")))?; + if !out.success() { + return Err(FbuildError::SerialError(format!( + "elevated powercfg failed: {}", + out.stderr.trim() + ))); + } + crate::output::result("USB selective suspend disabled; re-run `fbuild port doctor` to confirm"); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Both indices matter: disabling only AC leaves a laptop suspending + /// ports the moment it is unplugged. + #[test] + fn fix_covers_both_ac_and_dc() { + let cmds = suspend_fix_commands(); + assert_eq!(cmds.len(), 2); + assert!(cmds.iter().any(|c| c.contains(&"-setacvalueindex".into()))); + assert!(cmds.iter().any(|c| c.contains(&"-setdcvalueindex".into()))); + for c in &cmds { + assert_eq!(c.last().unwrap(), "0", "must disable, not enable"); + assert!(c.contains(&USB_SUBGROUP_GUID.to_string())); + assert!(c.contains(&SELECTIVE_SUSPEND_GUID.to_string())); + } + } + + /// Idempotence: an already-disabled host must produce no plan, so `--fix` + /// never raises a UAC prompt for a no-op. + #[test] + fn fix_plan_is_empty_when_already_disabled() { + let plan = render_fix_plan(&suspend_fix_commands(), true); + assert!(plan.contains("nothing to do"), "got: {plan}"); + assert!(!plan.contains("would run"), "got: {plan}"); + } + + /// A host-wide change must show exactly what it will run, and how to undo it. + #[test] + fn fix_plan_shows_commands_and_how_to_revert() { + let plan = render_fix_plan(&suspend_fix_commands(), false); + assert!(plan.contains("would run, elevated"), "got: {plan}"); + assert!(plan.contains("-setacvalueindex"), "got: {plan}"); + assert!(plan.contains("revert"), "got: {plan}"); + } +} diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index f9bcb568..fb709c72 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -89,7 +89,7 @@ pub fn run_port(action: PortAction) -> Result<()> { hub, } => { if fix || dry_run { - super::port_doctor::run_fix(dry_run, yes, no_elevate) + super::port_doctor_fix::run_fix(dry_run, yes, no_elevate) } else { super::port_doctor::run(port.as_deref(), hub.as_deref(), json) }