fix(device): don't abort the ioreg scan on one unparseable id - #97
Conversation
Adds an `ioreg -p IOUSB -l` fallback for USB presence, because `system_profiler SPUSBDataType` returns zero bytes with exit 0 on some macOS builds — reproduced on macOS 27, where it is the only working path. The first cut of the ioreg parser had three problems, all fixed here: - `?` on a failed `parse::<u16>()` inside the loop returned `None` for the whole scan, so one device with an unparseable `idProduct` hid every device listed after it — including the Micro. - `current_vid` was never cleared, so one device's vendor id could pair with the next device's product id. - It assumed `idVendor` precedes `idProduct`, an ordering ioreg's property dictionary does not promise. Parsing is now scoped per `+-o` device node, mirroring how `match_usb_text` already scopes to a record block, since the two ids only mean anything together. Also tolerates a `0x` form rather than reading a hex listing as "nothing attached". Tested: `cargo test -p mb-device` (20 tests, 4 new covering the malformed-id abort, cross-device pairing, reversed property order, and hex ids). Not yet exercised against attached hardware — no Codex Micro available, and the IOUSB plane lists no devices without one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesmacOS USB probe
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProbeUsbMicro
participant SystemProfiler
participant Ioreg
participant ProbeResult
ProbeUsbMicro->>SystemProfiler: request USB text
SystemProfiler-->>ProbeUsbMicro: return output
ProbeUsbMicro->>Ioreg: request USB text when unmatched
Ioreg-->>ProbeUsbMicro: return output
ProbeUsbMicro->>ProbeResult: return present or absent
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves macOS USB presence detection for the Codex Micro by adding an ioreg -p IOUSB -l fallback when system_profiler SPUSBDataType returns empty output, and by tightening ioreg parsing to avoid cross-device VID/PID pairing and to tolerate malformed/hex IDs.
Changes:
- Add
ioreg-based probe fallback whensystem_profileris empty or fails to detect the device. - Implement node-scoped
ioregparsing for VID/PID matching (with hex tolerance) plus name-based fallback. - Add unit tests covering multiple
ioregparsing edge cases.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let line = line.trim(); | ||
| // A malformed value must skip that property, never abort the scan: the | ||
| // Codex Micro may be listed after whatever failed to parse. | ||
| if let Some(rest) = line.strip_prefix("\"idvendor\" =") { | ||
| vid = vid.or_else(|| parse_ioreg_id(rest)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mb-device/src/probe.rs`:
- Around line 164-178: Update ioreg_usb_text to invoke ioreg with the same
bounded-process mechanism used by system_profiler_usb_text, preserving its
existing arguments and success/output handling while ensuring stalled processes
terminate instead of blocking the USB probe indefinitely.
🪄 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: CHILL
Plan: Pro
Run ID: 47ba3163-821d-4767-869c-bba626a8c9ef
📒 Files selected for processing (1)
crates/mb-device/src/probe.rs
| #[cfg(target_os = "macos")] | ||
| fn ioreg_usb_text() -> Option<String> { | ||
| use std::process::{Command, Stdio}; | ||
|
|
||
| let output = Command::new("/usr/sbin/ioreg") | ||
| .args(["-p", "IOUSB", "-l"]) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::null()) | ||
| .output() | ||
| .ok()?; | ||
| if !output.status.success() { | ||
| return None; | ||
| } | ||
| Some(String::from_utf8_lossy(&output.stdout).into_owned()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the ioreg invocation.
Command::output() waits indefinitely. When system_profiler fails/returns empty, a stalled ioreg now blocks the USB presence probe forever. Apply the same bounded-process behavior used for system_profiler_usb_text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mb-device/src/probe.rs` around lines 164 - 178, Update ioreg_usb_text
to invoke ioreg with the same bounded-process mechanism used by
system_profiler_usb_text, preserving its existing arguments and success/output
handling while ensuring stalled processes terminate instead of blocking the USB
probe indefinitely.
Hold — this PR's premise is superseded by v0.3.9I opened this against a stale local
So the bug this PR fixes — An Recommendation: close. The parser-correctness work here (per- One thing worth salvaging separately
let mut child = Command::new("system_profiler") // probe.rs:207This PR hardened that to 🤖 Generated with Claude Code |
Why
system_profiler SPUSBDataTypereturns zero bytes with exit 0 on some macOSbuilds, so USB presence detection silently found nothing. Reproduced on macOS 27,
where
ioregis the only path that works at all.This adds the
ioreg -p IOUSB -lfallback, and fixes three bugs in the parser.The three parser bugs
?on a failedparse::<u16>()inside the loopidProductreturnedNonefor the whole scan, hiding every device listed after it — including the Microcurrent_vidnever clearedidVendorprecedesidProductioreg's property dictionary does not promise that orderParsing is now scoped per
+-odevice node, mirroring howmatch_usb_textalready scopes to a record block — the two ids only mean anything together. Also
tolerates a
0xform, so a hex listing isn't read as "nothing attached".How this was tested
cargo test -p mb-device— 20 tests, 4 new, one per failure mode:malformed-id abort, cross-device id pairing, reversed property order, hex ids.
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warningsclean.system_profiler SPUSBDataType→ 0 bytes,exit 0. The
IOUSBplane is healthy (three XHCI controllers listed).Not verified
No hardware. With no Codex Micro attached the IOUSB plane lists no devices, so
the parser has not been run against a real listing. The unit tests use captured
ioregshapes. Worth a manual check with the device plugged in before relying ondetection.
🤖 Generated with Claude Code
Summary by CodeRabbit