dylint: code-quality gotcha sweep (#826) — 5 new lints + websocket hardening#833
Merged
Conversation
Adopt zccache's two PathBuf/tempdir lints into fbuild with allowlists matching the fbuild source tree: - ban_std_pathbuf: 193-entry allowlist of legacy PathBuf call sites; steers new code at fbuild_core::path::NormalizedPath. - ban_unrooted_tempdir: 92-entry allowlist of legacy unrooted tempdir call sites; steers new code at fbuild_paths::get_cache_root() and the tempfile::*_in(...) variants. Both lints are ON and deny-by-default for new code. Target state is zero allowlist entries; the lists shrink as migrations land. Add the dylint crate dirs to the crate_guard allowlist so future edits to those Cargo.tomls aren't blocked by the monocrate hook. Refs #826, #436, #437, #282.
Add three lints that enforce fbuild's own architectural rules:
- ban_direct_serialport: bans `serialport::*` references outside
crates/fbuild-serial/ so the Windows USB-CDC contract stays in one
place. Legacy allowlist covers fbuild-cli diagnostics, the daemon
device manager, and the fbuild-deploy bootloader paths.
- ban_file_based_locks: bans `OpenOptions::create_new(true)`, the
`fs2::FileExt` API, and `libc::flock` / `nix::flock`. Allowlist is
empty — fbuild has no file-based locks per CLAUDE.md "Key
Constraints"; this locks the invariant in.
- ban_deploy_tool_direct_invocation: bans `Command::new("esptool" |
"esptool.py" | "avrdude" | "picotool" | "dfu-util" | "pyocd")`
outside crates/fbuild-deploy/. Pattern-matches the binary-name
string literal; complements ban_raw_subprocess (which scopes by
spawn shape, not binary).
All three follow the ban_raw_subprocess skeleton: pinned nightly,
file-path scope (crates/*/src/), file-level allowlist via include_str!,
forward-slash normalized. Each carries unit tests for the scope/dir
helpers.
Refs #826, #694.
The WebSocket error-reply paths in `handle_serial_ws` previously panicked via `serde_json::to_string(&err_msg).unwrap()` if the JSON serializer ever failed. While `SerialServerMessage` always serializes cleanly today, a panic in the cold error path tears down the entire WebSocket — exactly the wrong failure mode for a code path whose only job is to surface an error to the client. Add a small `serialize_or_fallback` helper that returns a hardcoded JSON error frame on serializer failure, and route the 5 affected `Message::Text(...)` constructions through it (open_port failure, unexpected-message-shape, parse-error, attach_reader=None, attached-confirmation send). The 3 remaining `to_string(...).unwrap()` calls at lines ~441/450/467 are on normal data-forwarding paths (not error-reply paths) and are out of scope for this fix; #826 only flagged the error-reply ones as a daemon-stability hazard. Refs #826.
cargo metadata refuses to operate on dylint sub-crates that are neither members nor excluded; add the 5 new lint crates from #826 to the existing exclude list (alongside ban_raw_subprocess). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 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 Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
✨ 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
Jun 29, 2026
Adds five new dylints, completing the #826 audit: - ban_process_exit_outside_main — `std::process::exit` only in `crates/*/src/main.rs` and `crates/*/src/bin/*.rs`. Skipping destructors leaks temp files, kills containment guards, and truncates in-flight HTTP/WS responses. Legacy CLI subcommand dispatchers (build/deploy/dispatch/lnk/pio/purge/reset/serial_probe/ show) exempted via src/allowlist.txt. - ban_unwrap_in_daemon_handlers — `.unwrap()` banned in `crates/fbuild-daemon/src/handlers/**`. Test files (*tests*.rs) and `#[cfg(test)]` modules are exempt; lint walks owner chain to detect the cfg(test) ancestor. Three legacy production sites (websockets.rs, operations/build.rs, operations/deploy.rs) are file-level allowlisted with follow-up notes. - cli_no_build_deploy_direct_use — `fbuild-cli` is a thin HTTP client to the daemon; importing `fbuild_build::*` / `fbuild_deploy::*` is reserved for diagnostic subcommands (args/bloat_lookup/graph_cmd/ symbols_cmd/compile_many/reset). - require_multi_thread_flavor_when_spawning — `#[tokio::test]` on an async fn that calls `tokio::spawn` in its body must specify `flavor = "multi_thread"`, otherwise spawned tasks deadlock under the default `current_thread` flavor. Two legacy files (handlers/websockets_tests.rs, packages/install_lock.rs) allowlisted for follow-up migration. - ban_std_sync_mutex_in_async — `std::sync::Mutex` / `std::sync::RwLock` banned in `crates/fbuild-daemon/src/**` and `crates/fbuild-serial/src/**`. Holding the guard across `.await` starves the Tokio worker; a panic poisons the lock for every subsequent caller. Test files and `#[cfg(test)]` modules exempt; existing synchronous-discipline uses (context.rs, device_manager.rs, status_manager.rs, handlers/operations/common.rs, manager.rs) allowlisted with inline justification. All five lints follow the existing pattern from PR #833: - Own crate under dylints/, own rust-toolchain.toml pin (nightly-2026-03-26), trailofbits/dylint at git rev 4bd91ce… - Workspace excludes the crate so the stable workspace build is unaffected - File-path-scoped detection via cx.sess().source_map() with slash-normalized matching - File-level allowlist.txt with inline justifications - Tests covering scope detection, allowlist suffix matching, and entry-point exemptions Three additional items from #826's Tier 4 are not lints — they're audit / process work tracked as separate follow-up issues: - Mocks-as-primary-test-surface audit (recommend CodeRabbit rules, not dylint) - Ignored-test bitrot policy (recommend weekly grep-count to tracking issue) - Initializer-order lints (ban_env_var_set_after_import, require_oncelock_install_before_use) — need global flow analysis out of scope for this PR Test plan: - soldr cargo check --workspace --all-targets ✓ - soldr cargo clippy --workspace --all-targets -- -D warnings ✓ - soldr cargo test --workspace --no-fail-fast ✓ (all pass) - `cargo dylint --all` validation deferred to CI on Ubuntu — the Windows host's Chocolatey rust shadow blocks local invocation (filed as zackees/soldr#1059) See #826 for the original gotcha-sweep tracking issue and PR #833 for the first five lints this builds on. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 29, 2026
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.
Summary
Implements the code-quality dylint sweep from #826. Lands 5 new dylint crates + a small websockets error-reply hardening as one PR (4 commits).
What's in this PR
Two direct ports from zccache (Tier 1):
ban_std_pathbuffbuild_core::path::NormalizedPathinstead ofstd::path::PathBuf"ban_unrooted_tempdirTempDir::new()/tempdir()land in$TMPDIRwhich on Windows hits MAX_PATH and on CI may be tmpfs/RAM-disk"Three net-new fbuild-specific lints (Tier 2):
ban_direct_serialportban_file_based_locksban_deploy_tool_direct_invocationTier 3 fix (partial):
crates/fbuild-daemon/src/handlers/websockets.rslines 119/131/140/173/187 —serde_json::to_string(&err_msg).unwrap()in error-reply paths replaced with aserialize_or_fallback()helper that returns a hardcoded JSON error frame on serialization failure. Prevents a malformed message struct from crashing the WebSocket session (and potentially the daemon) inside the path that's supposed to report errors.Build glue:
Cargo.tomlworkspaceexcludeextended for the 5 new lint crates (cargo metadata refuses to operate otherwise).ci/hooks/crate_guard.pyallowlist expanded for the new dylint Cargo.toml files.What's NOT in this PR (deferred to follow-ups)
From #826:
Test plan
Rollout
🤖 Generated with Claude Code