feat(serial): Linux sysfs USB health/topology enrichment (#1091) - #1207
Merged
Conversation
- New platform-neutral sysfs_usb module: pure functions over a rooted directory (live root /sys/bus/usb/devices is the only cfg(target_os=linux) piece), so all parsing is fixture-tested on any OS. Interfaces are identified structurally (subdir with bInterfaceClass) rather than by the colon-named Linux dir, keeping fixtures Windows-legal while staying faithful to real sysfs layout. - Health mapping is conservative per #1149's invariants: concrete signals only (unauthorized / unconfigured / CDC interface without a bound driver -> PresentProblem with documented Linux-only codes; healthy requires a bound tty interface); everything ambiguous stays Unknown, never fabricated. Windows shapes are not reused: a distinct LinuxUsbProblemDevice diagnostics type + sibling present_usb_problem_devices_linux(); existing APIs unchanged on all platforms. behind_external_hub mirrors the Windows ancestry semantics via port-path depth. - macOS explicitly unchanged (IOKit needs a mac host; documented). Part of #1091 (Linux implementation + fixtures; macOS and real-device validation runs remain). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zackees
added a commit
that referenced
this pull request
Jul 31, 2026
… unused_mut, rustdoc links main is red on six checks. They are unrelated to each other and accumulated because the workflows that run them are `pull_request`-triggered, so nothing re-ran them on main after the merges that broke them (#1197, #1206, #1207). Found while verifying an unrelated dependency bump (#1215); every failure there reproduced identically on main. Dylint: drop `crates/fbuild-cli/src/cli/clangd_config.rs` from ban_unrooted_tempdir's allowlist -- the file was deleted in #1197. Lint subprocess spawns: annotate three deliberate direct spawns with `allow-direct-spawn:` markers (clangd test driver, interactive gdb that must inherit terminal stdio, detached editor launch that must outlive the CLI). The scanner only inspects the hit line and the one line directly above it, so the marker must be a single line -- a wrapped comment silently fails to register. This also fixes Check (ubuntu-latest), which runs the same script. Check (macos-latest): `ports.rs` bound `let mut ports` unconditionally while only `cfg(target_os = "linux")` mutates it, so every other unix target tripped `-D unused-mut`. The `mut` now exists only inside the Linux cfg via a shadowing block, rather than an `#[allow]`. Documentation: six rustdoc errors across three crates. URL templates such as `?query=<optional filter>` parsed as unclosed HTML tags; several intra-doc links pointed at `cfg(target_os = "linux")` items that do not exist to resolve against when docs build on a non-Linux host; three more pointed at private items. Unresolvable links become code spans, each with a short note on why it is deliberately not a link. CH32V006 (`build / build`) is also red on main but is a vendor-C compile error in the openwch Arduino core, not CI hygiene, and is left for a targeted fix. Verified locally on Windows, all exit 0: check_dylint_allowlists.py, find_direct_subprocess.py --fail, cargo doc with RUSTDOCFLAGS=-D warnings, clippy -D warnings, cargo fmt --check, and `bash test` (77 suites, 0 failed). The macOS fix could not be verified locally: cross-compiling fbuild-serial to x86_64-apple-darwin fails on ring's build script needing a macOS `cc`. The cfg-shadowing pattern was verified in an isolated cargo project (clean under -D warnings with the cfg both on and off); Check (macos-latest) on this PR is the real proof. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Jul 31, 2026
Fallout from repairing Dylint. With `dylint_linting` pinned back to 6.0.1 the driver builds again, so the lints actually *ran* for the first time in a while -- and `ban_std_pathbuf` immediately flagged three real violations in `crates/fbuild-serial/src/sysfs_usb.rs`, a file added by #1207. They went unnoticed because Dylint was failing at two earlier layers (a stale allowlist path, then the 6.0.2 driver), so the lint pass was never reached. Migrated rather than allowlisted. The allowlist's own header is explicit: "New files MUST NOT be added here. The target state is zero entries." `sysfs_usb.rs` is a new file, and exempting it would have quietly defeated the lint that was just repaired. - `scan_usb_devices`'s `device_dirs` is now `Vec<(String, NormalizedPath)>`. - `live_root()` returns `NormalizedPath`. Callers are unchanged: they pass `&root` into functions taking `&Path`, which `Deref` coercion handles. Dropping `PathBuf` from the library left the top-level import unused, which is an error under CI's `RUSTFLAGS="-D warnings"`, not a warning. The test module still builds fixture paths with it, so the import moved there. Verified: `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --all --check`, and `bash test` (77 suites, 0 failed) all green; workspace clippy confirms the `live_root` signature change breaks no callers. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Jul 31, 2026
Pinning dylint back to =6.0.1 made the driver build again, so the lints actually ran for the first time in a while and reported 34 violations across three crates. None are new: they accumulated in #1197, #1206 and #1207 while Dylint was failing at earlier steps (a stale allowlist path, then the 6.0.2 driver) and never reached the lint pass. Handled by category rather than uniformly. ban_std_pathbuf (30) -- migrated to `fbuild_core::path::NormalizedPath` in `fbuild-cli` (`ide.rs`, `debug.rs`, `ide_debug.rs`, `clangd_config/{mod, vscode,zed}.rs`), `fbuild-daemon` (`handlers/libraries.rs`) and the `sysfs_usb.rs` test fixtures. Not allowlisted: that allowlist's header states "New files MUST NOT be added here. The target state is zero entries", and exempting brand-new files would hollow out the gate that was just repaired. `NormalizedPath` derefs to `Path`, so call sites taking `&Path` are unchanged; `to_path_buf()` appears only where an external API (`BuildLayout::new`) demands an owned `PathBuf`. ban_std_fs_in_async (2) -- `handlers/libraries.rs::installed_dir_names` now uses async `fbuild_core::fs::read_dir` (the sanctioned re-export of `tokio::fs`), which makes it and `build_library_entries` async along with their three unit tests. It also switches the directory test from `Path::is_dir()` to `entry.file_type().await`: `is_dir()` is a blocking `stat` on the async worker, so keeping it would have satisfied the lint's letter while preserving exactly the stall the lint exists to prevent (#844). ban_raw_subprocess (2) -- allowlisted with justifications, which is what the lint's own diagnostic offers for genuinely-justified sites. Both are correct as raw spawns: `fbuild debug` hands the terminal to gdb, so the child must inherit this process's stdio rather than be captured; and `fbuild ide` launches the editor detached so it OUTLIVES the CLI, which is the opposite of what a containment group guarantees. Verified: `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --all --check`, and `bash test` (77 suites, 0 failed) all green. Dylint itself could not be run locally to pre-verify -- the pinned nightly and `cargo-dylint` are present, but `dylint-link` fails to link the lint cdylibs on this Windows host. CI is the proof for that gate. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Jul 31, 2026
Pinning dylint back to =6.0.1 made the driver build again, so the lints actually ran for the first time in a while and reported 34 violations across three crates. None are new: they accumulated in #1197, #1206 and #1207 while Dylint was failing at earlier steps (a stale allowlist path, then the 6.0.2 driver) and never reached the lint pass. Handled by category rather than uniformly. ban_std_pathbuf (30) -- migrated to `fbuild_core::path::NormalizedPath` in `fbuild-cli` (`ide.rs`, `debug.rs`, `ide_debug.rs`, `clangd_config/{mod, vscode,zed}.rs`), `fbuild-daemon` (`handlers/libraries.rs`) and the `sysfs_usb.rs` test fixtures. Not allowlisted: that allowlist's header states "New files MUST NOT be added here. The target state is zero entries", and exempting brand-new files would hollow out the gate that was just repaired. `NormalizedPath` derefs to `Path`, so call sites taking `&Path` are unchanged; `to_path_buf()` appears only where an external API (`BuildLayout::new`) demands an owned `PathBuf`. ban_std_fs_in_async (2) -- `handlers/libraries.rs::installed_dir_names` now uses async `fbuild_core::fs::read_dir` (the sanctioned re-export of `tokio::fs`), which makes it and `build_library_entries` async along with their three unit tests. It also switches the directory test from `Path::is_dir()` to `entry.file_type().await`: `is_dir()` is a blocking `stat` on the async worker, so keeping it would have satisfied the lint's letter while preserving exactly the stall the lint exists to prevent (#844). ban_raw_subprocess (2) -- allowlisted with justifications, which is what the lint's own diagnostic offers for genuinely-justified sites. Both are correct as raw spawns: `fbuild debug` hands the terminal to gdb, so the child must inherit this process's stdio rather than be captured; and `fbuild ide` launches the editor detached so it OUTLIVES the CLI, which is the opposite of what a containment group guarantees. Verified: `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --all --check`, and `bash test` (77 suites, 0 failed) all green. Dylint itself could not be run locally to pre-verify -- the pinned nightly and `cargo-dylint` are present, but `dylint-link` fails to link the lint cdylibs on this Windows host. CI is the proof for that gate. Co-Authored-By: Claude <noreply@anthropic.com> Also widens the cargo-cache .gitignore entries to `**/`-prefixed forms. The existing patterns contain a slash, so git anchors them to the repo root and they never covered nested crates; running any cargo command inside `dylints/<lint>/` (each is its own workspace) materializes a full registry cache there that a `git add -A` will stage.
zackees
added a commit
that referenced
this pull request
Jul 31, 2026
Fallout from repairing Dylint. With `dylint_linting` pinned back to 6.0.1 the driver builds again, so the lints actually *ran* for the first time in a while -- and `ban_std_pathbuf` immediately flagged three real violations in `crates/fbuild-serial/src/sysfs_usb.rs`, a file added by #1207. They went unnoticed because Dylint was failing at two earlier layers (a stale allowlist path, then the 6.0.2 driver), so the lint pass was never reached. Migrated rather than allowlisted. The allowlist's own header is explicit: "New files MUST NOT be added here. The target state is zero entries." `sysfs_usb.rs` is a new file, and exempting it would have quietly defeated the lint that was just repaired. - `scan_usb_devices`'s `device_dirs` is now `Vec<(String, NormalizedPath)>`. - `live_root()` returns `NormalizedPath`. Callers are unchanged: they pass `&root` into functions taking `&Path`, which `Deref` coercion handles. Dropping `PathBuf` from the library left the top-level import unused, which is an error under CI's `RUSTFLAGS="-D warnings"`, not a warning. The test module still builds fixture paths with it, so the import moved there. Verified: `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --all --check`, and `bash test` (77 suites, 0 failed) all green; workspace clippy confirms the `live_root` signature change breaks no callers. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Jul 31, 2026
Pinning dylint back to =6.0.1 made the driver build again, so the lints actually ran for the first time in a while and reported 34 violations across three crates. None are new: they accumulated in #1197, #1206 and #1207 while Dylint was failing at earlier steps (a stale allowlist path, then the 6.0.2 driver) and never reached the lint pass. Handled by category rather than uniformly. ban_std_pathbuf (30) -- migrated to `fbuild_core::path::NormalizedPath` in `fbuild-cli` (`ide.rs`, `debug.rs`, `ide_debug.rs`, `clangd_config/{mod, vscode,zed}.rs`), `fbuild-daemon` (`handlers/libraries.rs`) and the `sysfs_usb.rs` test fixtures. Not allowlisted: that allowlist's header states "New files MUST NOT be added here. The target state is zero entries", and exempting brand-new files would hollow out the gate that was just repaired. `NormalizedPath` derefs to `Path`, so call sites taking `&Path` are unchanged; `to_path_buf()` appears only where an external API (`BuildLayout::new`) demands an owned `PathBuf`. ban_std_fs_in_async (2) -- `handlers/libraries.rs::installed_dir_names` now uses async `fbuild_core::fs::read_dir` (the sanctioned re-export of `tokio::fs`), which makes it and `build_library_entries` async along with their three unit tests. It also switches the directory test from `Path::is_dir()` to `entry.file_type().await`: `is_dir()` is a blocking `stat` on the async worker, so keeping it would have satisfied the lint's letter while preserving exactly the stall the lint exists to prevent (#844). ban_raw_subprocess (2) -- allowlisted with justifications, which is what the lint's own diagnostic offers for genuinely-justified sites. Both are correct as raw spawns: `fbuild debug` hands the terminal to gdb, so the child must inherit this process's stdio rather than be captured; and `fbuild ide` launches the editor detached so it OUTLIVES the CLI, which is the opposite of what a containment group guarantees. Verified: `cargo clippy --workspace --all-targets -- -D warnings`, `cargo fmt --all --check`, and `bash test` (77 suites, 0 failed) all green. Dylint itself could not be run locally to pre-verify -- the pinned nightly and `cargo-dylint` are present, but `dylint-link` fails to link the lint cdylibs on this Windows host. CI is the proof for that gate. Co-Authored-By: Claude <noreply@anthropic.com> Also widens the cargo-cache .gitignore entries to `**/`-prefixed forms. The existing patterns contain a slash, so git anchors them to the repo root and they never covered nested crates; running any cargo command inside `dylints/<lint>/` (each is its own workspace) materializes a full registry cache there that a `git add -A` will stage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the Linux slice of #1091: sysfs-based USB health/topology enrichment, designed so every line of parsing logic is fixture-testable on any OS.
sysfs_usbparses a rooted directory tree; onlylive_root()(/sys/bus/usb/devices) is Linux-gated. Interfaces are detected structurally (subdirectory containingbInterfaceClass) instead of by Linux's colon-containing interface names — colons are illegal in NTFS filenames, so this keeps fixtures Windows-legal while matching real sysfs nesting.Unknownnever fabricated):PresentProblemonly on concrete signals (authorized=0, unconfigured, CDC interface with no bound driver) using documented Linux-only problem codes explicitly unrelated to WindowsCM_PROB_*; healthy requires a bound tty interface; everything ambiguous staysUnknown. The WindowsUsbProblemDeviceshape is not abused — Linux gets a distinct diagnostics-onlyLinuxUsbProblemDevice+present_usb_problem_devices_linux(); existing APIs behave identically on all platforms.behind_external_hubmirrors the Windows ancestry classification via port-path depth (1-1root-attached vs1-1.4behind a hub).Unknown— IOKit work requires a Mac host (documented in the module).Validation (local)
soldr cargo test -p fbuild-serial→ 176 + 7 passed (15 new fixture tests: healthy CDC, hub vs root topology, unauthorized, unconfigured, driverless CDC interface, malformed/partial entries never panic, RP2040-typical synthetic VID/PID fixture, unowned tty stays Unknown); clippy-D warningsclean; dependents (fbuild-daemon,fbuild-cli) check clean; fullbash testbefore merge.Part of #1091 — remaining: macOS (IOKit) and the real-device validation runs on both platforms.
🤖 Generated with Claude Code