dylint: extended gotcha sweep (#826 — out-of-scope items)#837
Merged
Conversation
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>
|
Warning Review limit reached
Next review available in: 44 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 selected for processing (28)
✨ 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 |
40 tasks
zackees
added a commit
that referenced
this pull request
Jun 29, 2026
Releases the post-#813 / #802 / #826 / #844 work landed across: - PR #843 — workspace-wide timeout sweep (#802 family) - PR #837, #849, #850, #851 — internal-bridges + dylint sweep (#826 + #844) - PR #842 — async migration follow-ups (#813) 15 dylints now ship in dylints/ (4 from earlier, 10 from #826/#844, plus the renamed ban_unwrap_in_production). 7 internal bridge APIs: fbuild_core::{http, fs, time, channel, path::canonicalize_existing}, fbuild_paths::temp_subdir, fbuild_cli::output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Adds five new dylints, completing the #826 gotcha-sweep audit. Builds on PR #833 (which landed the first five lints + the websocket hardening). All five new lints follow the same pattern: own crate under
dylints/, ownrust-toolchain.toml(nightly-2026-03-26), trailofbits/dylint git rev4bd91ce…, file-level allowlist with inline justifications.New lints
ban_process_exit_outside_mainstd::process::exitoutsidecrates/*/src/main.rsandcrates/*/src/bin/*.rsskips destructors — temp files leak, containment guards don't run, in-flight HTTP/WS responses get truncated.build,deploy,dispatch,lnk,pio,purge,reset,serial_probe,show)ban_unwrap_in_daemon_handlers.unwrap()inside an fbuild-daemon handler crashes the daemon and disconnects every connected client. Test files (*tests*.rs) and#[cfg(test)]modules are exempt (lint walks the owner chain).websockets.rs,operations/build.rs,operations/deploy.rs) forserde_json::to_string/Response::builder().body()/ static-table lookups that can't fail in practice — follow-up.unwrap_or_default()/.expect("…")migrationcli_no_build_deploy_direct_usefbuild-cliis a thin HTTP client — importingfbuild_build::*/fbuild_deploy::*bypasses the daemon.crates/CLAUDE.md§"Diagnostic subcommand exception" (args,bloat_lookup,graph_cmd,symbols_cmd,compile_many,reset)require_multi_thread_flavor_when_spawning#[tokio::test]on an async fn that callstokio::spawnin its body must specifyflavor = "multi_thread", otherwise spawned tasks deadlock under the defaultcurrent_threadflavor.handlers/websockets_tests.rs,packages/install_lock.rs) — both rely on single-threaded ordering and need an explicit-synchronization reworkban_std_sync_mutex_in_asyncstd::sync::Mutex/std::sync::RwLockincrates/fbuild-daemon/src/**orcrates/fbuild-serial/src/**— holding the guard across.awaitstarves the Tokio worker; a panic poisons the lock. Test files and#[cfg(test)]modules exempt.context.rs,device_manager.rs,status_manager.rs,handlers/operations/common.rs,manager.rs) — each with inline justificationFollow-up issues filed separately (not lints)
audit: replace mock-based tests with integration tests …— best handled via CodeRabbit.coderabbit.yamlrules + human review, not dylintprocess: ignored-test policy + bitrot audit (#826 followup)— recommend a weekly grep-count posted to a tracking issuedylint: initializer-order lints (ban_env_var_set_after_import, require_oncelock_install_before_use) — design and implementation— both need global flow analysis (initializer ordering) that's out of scope for a single PRTest plan
soldr cargo check --workspace --all-targetssoldr cargo clippy --workspace --all-targets -- -D warningssoldr cargo test --workspace --no-fail-fast(all pass)cargo dylint --all— deferred to CI on Ubuntu; the Windows host's Chocolatey rust shadow blocks local invocation (filed as Windows: Chocolatey cargo shadows rustup proxy; defeats per-crate rust-toolchain.toml for cargo-dylint and similar extensions zackees/soldr#1059). CI is the source of truth.🤖 Generated with Claude Code