diff --git a/Cargo.toml b/Cargo.toml index 284b7219..53846ef8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ exclude = [ "dylints/ban_file_based_locks", "dylints/ban_deploy_tool_direct_invocation", "dylints/ban_process_exit_outside_main", - "dylints/ban_unwrap_in_daemon_handlers", + "dylints/ban_unwrap_in_production", "dylints/cli_no_build_deploy_direct_use", "dylints/require_multi_thread_flavor_when_spawning", "dylints/ban_std_sync_mutex_in_async", diff --git a/ci/hooks/crate_guard.py b/ci/hooks/crate_guard.py index 033d4b86..48797cb7 100644 --- a/ci/hooks/crate_guard.py +++ b/ci/hooks/crate_guard.py @@ -66,7 +66,7 @@ "dylints/ban_file_based_locks", "dylints/ban_deploy_tool_direct_invocation", "dylints/ban_process_exit_outside_main", - "dylints/ban_unwrap_in_daemon_handlers", + "dylints/ban_unwrap_in_production", "dylints/cli_no_build_deploy_direct_use", "dylints/require_multi_thread_flavor_when_spawning", "dylints/ban_std_sync_mutex_in_async", diff --git a/crates/fbuild-build/src/compiler.rs b/crates/fbuild-build/src/compiler.rs index 1a173283..07b19094 100644 --- a/crates/fbuild-build/src/compiler.rs +++ b/crates/fbuild-build/src/compiler.rs @@ -424,7 +424,12 @@ fn looks_like_absolute_path(path: &Path, raw: &str) -> bool { fn compiler_identity(path: &Path) -> String { let cache = COMPILER_IDENTITY_CACHE.get_or_init(|| Mutex::new(HashMap::new())); - if let Some(identity) = cache.lock().unwrap().get(path).cloned() { + if let Some(identity) = cache + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(path) + .cloned() + { return identity; } @@ -437,7 +442,7 @@ fn compiler_identity(path: &Path) -> String { let identity = format!("{stem}\0{version}"); cache .lock() - .unwrap() + .unwrap_or_else(|e| e.into_inner()) .insert(path.to_path_buf(), identity.clone()); identity } diff --git a/crates/fbuild-build/src/esp32/orchestrator/build.rs b/crates/fbuild-build/src/esp32/orchestrator/build.rs index 905150c5..bbb1abfa 100644 --- a/crates/fbuild-build/src/esp32/orchestrator/build.rs +++ b/crates/fbuild-build/src/esp32/orchestrator/build.rs @@ -684,7 +684,9 @@ impl BuildOrchestrator for Esp32Orchestrator { }; // Unwrap build log and flush collected warnings - let mut build_log = build_log_mutex.into_inner().unwrap(); + let mut build_log = build_log_mutex + .into_inner() + .unwrap_or_else(|e| e.into_inner()); for w in core_result.warnings.iter().chain(&sketch_result.warnings) { crate::build_output::collect_warnings(w, &mut build_log); } diff --git a/crates/fbuild-build/src/pipeline/compile.rs b/crates/fbuild-build/src/pipeline/compile.rs index c6f3d7a7..134eabba 100644 --- a/crates/fbuild-build/src/pipeline/compile.rs +++ b/crates/fbuild-build/src/pipeline/compile.rs @@ -33,7 +33,7 @@ pub async fn compile_sources( ) .await?; if !result.warnings.is_empty() { - let mut log = build_log.lock().unwrap(); + let mut log = build_log.lock().unwrap_or_else(|e| e.into_inner()); for w in &result.warnings { crate::build_output::collect_warnings(w, &mut log); } @@ -106,7 +106,7 @@ pub async fn compile_local_libraries( })?; library_objects.extend(result.objects); if !result.warnings.is_empty() { - let mut log = build_log.lock().unwrap(); + let mut log = build_log.lock().unwrap_or_else(|e| e.into_inner()); for w in &result.warnings { crate::build_output::collect_warnings(w, &mut log); } diff --git a/crates/fbuild-build/src/pipeline/sequential.rs b/crates/fbuild-build/src/pipeline/sequential.rs index 54c727a7..bc661e63 100644 --- a/crates/fbuild-build/src/pipeline/sequential.rs +++ b/crates/fbuild-build/src/pipeline/sequential.rs @@ -230,7 +230,9 @@ pub async fn run_sequential_build_with_libs( // Unwrap the build log Mutex back into the context for the remaining // single-threaded phases (link, result assembly). - ctx.build_log = build_log_mutex.into_inner().unwrap(); + ctx.build_log = build_log_mutex + .into_inner() + .unwrap_or_else(|e| e.into_inner()); // Project-as-library: compile project root's src/ as an archive when // building an example sketch from a library project (e.g. FastLED examples). diff --git a/crates/fbuild-cli/src/cli/bloat_lookup.rs b/crates/fbuild-cli/src/cli/bloat_lookup.rs index fbba0ed0..66347ab3 100644 --- a/crates/fbuild-cli/src/cli/bloat_lookup.rs +++ b/crates/fbuild-cli/src/cli/bloat_lookup.rs @@ -73,7 +73,10 @@ pub async fn run_bloat_lookup( }; let report = analyze_elf(cfg).await?; - let query_str = symbol.clone().or_else(|| symbol_mangled.clone()).unwrap(); + let query_str = symbol + .clone() + .or_else(|| symbol_mangled.clone()) + .expect("fbuild-cli: bloat_lookup guards that symbol or symbol_mangled is Some"); let query = match (&symbol, &symbol_mangled) { (Some(s), _) => SymbolQuery::SubstringDemangled(s), (None, Some(m)) => SymbolQuery::ExactMangled(m), diff --git a/crates/fbuild-cli/src/cli/clang_tools.rs b/crates/fbuild-cli/src/cli/clang_tools.rs index f7a653f8..95eea203 100644 --- a/crates/fbuild-cli/src/cli/clang_tools.rs +++ b/crates/fbuild-cli/src/cli/clang_tools.rs @@ -270,7 +270,10 @@ pub async fn run_iwyu( let emap = entry_map_arc.clone(); let verbose_flag = verbose; let handle = tokio::spawn(async move { - let _permit = sem.acquire().await.unwrap(); + let _permit = sem + .acquire() + .await + .expect("fbuild-cli: clang-tool semaphore is never closed before all tasks finish"); let src_path = src.as_ref().clone(); // Compute blake3 cache key from source content + compile entry @@ -570,7 +573,10 @@ pub async fn run_clang_tool( let pd = project_dir_arc.clone(); let extra = extra_owned.clone(); let handle = tokio::spawn(async move { - let _permit = sem.acquire().await.unwrap(); + let _permit = sem + .acquire() + .await + .expect("fbuild-cli: clang-tool semaphore is never closed before all tasks finish"); // allow-direct-spawn: parallel async fan-out (clang-tidy) in CLI binary. let mut cmd = tokio::process::Command::new(tool.as_ref()); cmd.arg("-p").arg(pd.as_ref()); diff --git a/crates/fbuild-cli/src/lib_select.rs b/crates/fbuild-cli/src/lib_select.rs index 0d68e593..393b9e4c 100644 --- a/crates/fbuild-cli/src/lib_select.rs +++ b/crates/fbuild-cli/src/lib_select.rs @@ -427,5 +427,9 @@ fn emit_json( "unresolved": sel.unresolved, }); - println!("{}", serde_json::to_string_pretty(&payload).unwrap()); + println!( + "{}", + serde_json::to_string_pretty(&payload) + .expect("fbuild-cli: lib-select JSON payload is built from primitives, serialization is infallible") + ); } diff --git a/crates/fbuild-cli/src/mcp/server.rs b/crates/fbuild-cli/src/mcp/server.rs index 31efa3da..e9213373 100644 --- a/crates/fbuild-cli/src/mcp/server.rs +++ b/crates/fbuild-cli/src/mcp/server.rs @@ -62,7 +62,9 @@ pub async fn run_mcp_server() -> i32 { continue; } - let id = request.id.unwrap(); + let id = request + .id + .expect("fbuild-cli: notification (id.is_none()) handled above; id is Some here"); let params = request.params.unwrap_or(Value::Null); let response = match request.method.as_str() { diff --git a/crates/fbuild-config/src/bin/enrich_boards.rs b/crates/fbuild-config/src/bin/enrich_boards.rs index 4ecadc81..d724b697 100644 --- a/crates/fbuild-config/src/bin/enrich_boards.rs +++ b/crates/fbuild-config/src/bin/enrich_boards.rs @@ -244,7 +244,13 @@ fn enrich_board(board_path: &Path, pio_dir: &Path) -> Result { let board_id = obj .get("id") .and_then(|v| v.as_str()) - .unwrap_or_else(|| board_path.file_stem().unwrap().to_str().unwrap()); + .unwrap_or_else(|| { + board_path + .file_stem() + .expect("fbuild-config: board JSON path always has a file stem") + .to_str() + .expect("fbuild-config: board JSON file stems are ASCII") + }); let platform = obj.get("platform").and_then(|v| v.as_str()).unwrap_or(""); if platform.is_empty() { @@ -340,7 +346,10 @@ fn main() { Err(e) => { eprintln!( " Error enriching {}: {e}", - board_path.file_stem().unwrap().to_string_lossy() + board_path + .file_stem() + .expect("fbuild-config: board JSON path always has a file stem") + .to_string_lossy() ); errors += 1; } diff --git a/crates/fbuild-config/src/ini_parser/parser.rs b/crates/fbuild-config/src/ini_parser/parser.rs index 7680ba4f..dc135d33 100644 --- a/crates/fbuild-config/src/ini_parser/parser.rs +++ b/crates/fbuild-config/src/ini_parser/parser.rs @@ -214,7 +214,8 @@ pub(super) fn substitute_vars( sections: &HashMap>, config: &HashMap, ) -> HashMap { - let re = Regex::new(r"\$\{([^}]+)\}").unwrap(); + let re = Regex::new(r"\$\{([^}]+)\}") + .expect("fbuild-config: ini variable substitution regex is a valid pattern"); let max_depth = 10; let mut result = config.clone(); diff --git a/crates/fbuild-daemon/src/handlers/operations/build.rs b/crates/fbuild-daemon/src/handlers/operations/build.rs index f06d22a6..33a52cee 100644 --- a/crates/fbuild-daemon/src/handlers/operations/build.rs +++ b/crates/fbuild-daemon/src/handlers/operations/build.rs @@ -575,7 +575,7 @@ pub async fn build( axum::response::Response::builder() .header("content-type", "application/x-ndjson") .body(body) - .unwrap() + .expect("fbuild-daemon: static NDJSON response builder cannot fail") .into_response() } else { // --- NON-STREAMING PATH (existing behavior) --- diff --git a/crates/fbuild-daemon/src/handlers/operations/deploy.rs b/crates/fbuild-daemon/src/handlers/operations/deploy.rs index 8be0d356..643b3a1b 100644 --- a/crates/fbuild-daemon/src/handlers/operations/deploy.rs +++ b/crates/fbuild-daemon/src/handlers/operations/deploy.rs @@ -734,7 +734,7 @@ pub async fn deploy( &deploy_board_overrides, Some(deploy_project.as_path()), ); - let avr_config = fbuild_build::avr::mcu_config::get_avr_config().unwrap(); + let avr_config = fbuild_build::avr::mcu_config::get_avr_config()?; let avrdude_params = fbuild_deploy::avr::AvrdudeParams { default_programmer: avr_config.avrdude.default_programmer.clone(), default_baud: avr_config.avrdude.default_baud.to_string(), diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index 979a14f8..5737b696 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -462,7 +462,11 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { current_index: last_index, }; if ws_sink - .send(Message::Text(serde_json::to_string(&coalesced).unwrap())) + .send(Message::Text( + serde_json::to_string(&coalesced).expect( + "fbuild-daemon: SerialServerMessage::Data serialization is infallible", + ), + )) .await .is_err() { @@ -471,7 +475,9 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { } } if ws_sink - .send(Message::Text(serde_json::to_string(&other).unwrap())) + .send(Message::Text(serde_json::to_string(&other).expect( + "fbuild-daemon: SerialServerMessage serialization is infallible", + ))) .await .is_err() { @@ -488,7 +494,9 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { current_index: last_index, }; if ws_sink - .send(Message::Text(serde_json::to_string(&coalesced).unwrap())) + .send(Message::Text(serde_json::to_string(&coalesced).expect( + "fbuild-daemon: SerialServerMessage::Data serialization is infallible", + ))) .await .is_err() { diff --git a/crates/fbuild-daemon/src/main.rs b/crates/fbuild-daemon/src/main.rs index f01fec57..5b680a06 100644 --- a/crates/fbuild-daemon/src/main.rs +++ b/crates/fbuild-daemon/src/main.rs @@ -151,8 +151,12 @@ async fn main() { .layer( tower_http::cors::CorsLayer::new() .allow_origin([ - "http://localhost".parse().unwrap(), - "http://127.0.0.1".parse().unwrap(), + "http://localhost" + .parse() + .expect("fbuild-daemon: 'http://localhost' is a valid CORS origin"), + "http://127.0.0.1" + .parse() + .expect("fbuild-daemon: 'http://127.0.0.1' is a valid CORS origin"), ]) .allow_methods(tower_http::cors::Any) .allow_headers(tower_http::cors::Any), diff --git a/crates/fbuild-deploy/src/esp32/image.rs b/crates/fbuild-deploy/src/esp32/image.rs index b95d7fd4..caa316ec 100644 --- a/crates/fbuild-deploy/src/esp32/image.rs +++ b/crates/fbuild-deploy/src/esp32/image.rs @@ -103,10 +103,16 @@ pub(super) fn resolve_esp_image_file_offset(firmware_bin: &[u8], load_addr: u32) "firmware.bin ended before segment header".to_string(), )); } - let seg_load_addr = - u32::from_le_bytes(firmware_bin[cursor..cursor + 4].try_into().unwrap()); - let seg_len = - u32::from_le_bytes(firmware_bin[cursor + 4..cursor + 8].try_into().unwrap()) as usize; + let seg_load_addr = u32::from_le_bytes( + firmware_bin[cursor..cursor + 4] + .try_into() + .expect("fbuild-deploy: 4-byte slice converts to [u8; 4] (bounds checked above)"), + ); + let seg_len = u32::from_le_bytes( + firmware_bin[cursor + 4..cursor + 8] + .try_into() + .expect("fbuild-deploy: 4-byte slice converts to [u8; 4] (bounds checked above)"), + ) as usize; let data_start = cursor + ESP_IMAGE_SEGMENT_HEADER_LEN; let data_end = data_start + seg_len; if data_end > firmware_bin.len() { @@ -185,8 +191,11 @@ pub(super) fn repair_esp_image_checksum_and_hash(image: &mut [u8]) -> Result<()> "firmware.bin ended before segment header".to_string(), )); } - let seg_len = - u32::from_le_bytes(image[cursor + 4..cursor + 8].try_into().unwrap()) as usize; + let seg_len = u32::from_le_bytes( + image[cursor + 4..cursor + 8] + .try_into() + .expect("fbuild-deploy: 4-byte slice converts to [u8; 4] (bounds checked above)"), + ) as usize; let data_start = cursor + ESP_IMAGE_SEGMENT_HEADER_LEN; let data_end = data_start + seg_len; if data_end > image.len() { diff --git a/crates/fbuild-header-scan/src/scanner.rs b/crates/fbuild-header-scan/src/scanner.rs index 3ab2841e..201a12de 100644 --- a/crates/fbuild-header-scan/src/scanner.rs +++ b/crates/fbuild-header-scan/src/scanner.rs @@ -135,11 +135,15 @@ pub fn scan(src: &str) -> Vec { && !prev_is_ident_continuation && is_raw_string_open(bytes, i) { - let open_quote = bytes[i..].iter().position(|&c| c == b'"').unwrap() + i; + let open_quote = bytes[i..] + .iter() + .position(|&c| c == b'"') + .expect("fbuild-header-scan: is_raw_string_open guarantees '\"' ahead") + + i; let paren = bytes[open_quote + 1..] .iter() .position(|&c| c == b'(') - .unwrap() + .expect("fbuild-header-scan: is_raw_string_open guarantees '(' after the opening quote") + open_quote + 1; raw_delim.clear(); diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index ec1c2d55..edbf78a4 100644 --- a/crates/fbuild-paths/src/lib.rs +++ b/crates/fbuild-paths/src/lib.rs @@ -342,7 +342,11 @@ pub fn find_firmware( /// Like `find_firmware()` but returns the directory, not the file. /// Useful when you need sibling files (bootloader.bin, partitions.bin). pub fn find_firmware_dir(project_dir: &Path, env_name: &str) -> Option { - find_firmware(project_dir, env_name, None).map(|p| p.parent().unwrap().to_path_buf()) + find_firmware(project_dir, env_name, None).map(|p| { + p.parent() + .expect("fbuild-paths: find_firmware always returns a file under a directory") + .to_path_buf() + }) } fn dirs_next() -> Option { diff --git a/crates/fbuild-python/src/async_serial_monitor.rs b/crates/fbuild-python/src/async_serial_monitor.rs index f0e5d8ed..f39f3d79 100644 --- a/crates/fbuild-python/src/async_serial_monitor.rs +++ b/crates/fbuild-python/src/async_serial_monitor.rs @@ -134,7 +134,8 @@ impl AsyncSerialMonitor { open_if_needed: true, pre_acquire_writer: true, }; - let attach_json = serde_json::to_string(&attach).unwrap(); + let attach_json = serde_json::to_string(&attach) + .expect("fbuild-python: ClientMessage::Attach serialization is infallible"); match tokio::time::timeout( HANDSHAKE_TIMEOUT, @@ -219,7 +220,8 @@ impl AsyncSerialMonitor { pyo3_async_runtimes::tokio::future_into_py(py, async move { if let Some(mut write) = ws_write_slot.lock().await.take() { - let detach = serde_json::to_string(&ClientMessage::Detach).unwrap(); + let detach = serde_json::to_string(&ClientMessage::Detach) + .expect("fbuild-python: ClientMessage::Detach serialization is infallible"); let _ = write.send(tungstenite::Message::Text(detach)).await; let _ = write.send(tungstenite::Message::Close(None)).await; } diff --git a/crates/fbuild-python/src/json_rpc.rs b/crates/fbuild-python/src/json_rpc.rs index 34f348cb..7b0c4b9c 100644 --- a/crates/fbuild-python/src/json_rpc.rs +++ b/crates/fbuild-python/src/json_rpc.rs @@ -105,7 +105,8 @@ pub(crate) async fn write_async( ws_read_slot: Arc>>, encoded: String, ) -> bool { - let msg = serde_json::to_string(&ClientMessage::Write { data: encoded }).unwrap(); + let msg = serde_json::to_string(&ClientMessage::Write { data: encoded }) + .expect("fbuild-python: ClientMessage::Write serialization is infallible"); { let mut guard = ws_write_slot.lock().await; diff --git a/crates/fbuild-python/src/serial_monitor.rs b/crates/fbuild-python/src/serial_monitor.rs index 76007c52..5ca487c6 100644 --- a/crates/fbuild-python/src/serial_monitor.rs +++ b/crates/fbuild-python/src/serial_monitor.rs @@ -84,7 +84,8 @@ impl SerialMonitor { open_if_needed: true, pre_acquire_writer: true, }; - let attach_json = serde_json::to_string(&attach).unwrap(); + let attach_json = serde_json::to_string(&attach) + .expect("fbuild-python: ClientMessage::Attach serialization is infallible"); let send_result = rt.block_on(async { tokio::time::timeout(HANDSHAKE_TIMEOUT, write.send(tungstenite::Message::Text(attach_json))) @@ -154,7 +155,8 @@ impl SerialMonitor { fn close_ws(&mut self) { if let (Some(rt), Some(ws_write)) = (&self.runtime, &self.ws_write) { - let detach = serde_json::to_string(&ClientMessage::Detach).unwrap(); + let detach = serde_json::to_string(&ClientMessage::Detach) + .expect("fbuild-python: ClientMessage::Detach serialization is infallible"); if let Ok(mut write) = ws_write.lock() { let _ = rt.block_on(write.send(tungstenite::Message::Text(detach))); let _ = rt.block_on(write.send(tungstenite::Message::Close(None))); @@ -254,7 +256,7 @@ impl SerialMonitor { while std::time::Instant::now() < deadline { let remaining = deadline - std::time::Instant::now(); let result = { - let mut read = ws_read.lock().unwrap(); + let mut read = ws_read.lock().unwrap_or_else(|e| e.into_inner()); // tokio::time::timeout MUST be constructed inside the // runtime context, otherwise it panics with "there is // no reactor running" because the Sleep future needs @@ -326,10 +328,11 @@ impl SerialMonitor { }; let encoded = base64::engine::general_purpose::STANDARD.encode(data.as_bytes()); - let msg = serde_json::to_string(&ClientMessage::Write { data: encoded }).unwrap(); + let msg = serde_json::to_string(&ClientMessage::Write { data: encoded }) + .expect("fbuild-python: ClientMessage::Write serialization is infallible"); { - let mut write = ws_write.lock().unwrap(); + let mut write = ws_write.lock().unwrap_or_else(|e| e.into_inner()); if rt .block_on(write.send(tungstenite::Message::Text(msg))) .is_err() @@ -339,7 +342,7 @@ impl SerialMonitor { } // Wait for write_ack - let mut read = ws_read.lock().unwrap(); + let mut read = ws_read.lock().unwrap_or_else(|e| e.into_inner()); let timeout = std::time::Duration::from_secs(5); // tokio::time::timeout must be created inside the runtime context. match rt.block_on(async { tokio::time::timeout(timeout, read.next()).await }) { @@ -434,9 +437,10 @@ impl SerialMonitor { return 0; }; - let msg = serde_json::to_string(&ClientMessage::GetInWaiting).unwrap(); + let msg = serde_json::to_string(&ClientMessage::GetInWaiting) + .expect("fbuild-python: ClientMessage::GetInWaiting serialization is infallible"); { - let mut write = ws_write.lock().unwrap(); + let mut write = ws_write.lock().unwrap_or_else(|e| e.into_inner()); if rt .block_on(write.send(tungstenite::Message::Text(msg))) .is_err() @@ -449,7 +453,7 @@ impl SerialMonitor { // streaming Data frames or Preempted notifications) can arrive // in front of the reply; ignore them and keep waiting for the // typed answer until the 2s deadline expires. - let mut read = ws_read.lock().unwrap(); + let mut read = ws_read.lock().unwrap_or_else(|e| e.into_inner()); let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); while std::time::Instant::now() < deadline { let remaining = deadline - std::time::Instant::now(); @@ -481,8 +485,9 @@ impl SerialMonitor { let (Some(rt), Some(ws_write)) = (&self.runtime, &self.ws_write) else { return; }; - let msg = serde_json::to_string(&ClientMessage::ClearBuffer).unwrap(); - let mut write = ws_write.lock().unwrap(); + let msg = serde_json::to_string(&ClientMessage::ClearBuffer) + .expect("fbuild-python: ClientMessage::ClearBuffer serialization is infallible"); + let mut write = ws_write.lock().unwrap_or_else(|e| e.into_inner()); let _ = rt.block_on(write.send(tungstenite::Message::Text(msg))); } @@ -599,7 +604,7 @@ impl SerialMonitor { while std::time::Instant::now() < deadline { let remaining = deadline - std::time::Instant::now(); let result = { - let mut read = ws_read.lock().unwrap(); + let mut read = ws_read.lock().unwrap_or_else(|e| e.into_inner()); // tokio::time::timeout must be created inside the runtime // context (otherwise: "there is no reactor running" panic). rt.block_on(async { tokio::time::timeout(remaining, read.next()).await }) diff --git a/crates/fbuild-serial/src/crash_decoder.rs b/crates/fbuild-serial/src/crash_decoder.rs index 93890691..daad119a 100644 --- a/crates/fbuild-serial/src/crash_decoder.rs +++ b/crates/fbuild-serial/src/crash_decoder.rs @@ -46,28 +46,40 @@ fn riscv_register_re() -> &'static Regex { static RE: OnceLock = OnceLock::new(); RE.get_or_init(|| { Regex::new(r"(?:MEPC|RA|SP|GP|TP|T[0-6]|S[0-9]|S1[01]|A[0-7])\s*:\s*(0x[0-9a-fA-F]+)") - .unwrap() + .expect("fbuild-serial: riscv_register_re static pattern is a valid regex") }) } fn xtensa_backtrace_re() -> &'static Regex { static RE: OnceLock = OnceLock::new(); - RE.get_or_init(|| Regex::new(r"Backtrace:\s*((?:0x[0-9a-fA-F]+:0x[0-9a-fA-F]+\s*)+)").unwrap()) + RE.get_or_init(|| { + Regex::new(r"Backtrace:\s*((?:0x[0-9a-fA-F]+:0x[0-9a-fA-F]+\s*)+)") + .expect("fbuild-serial: xtensa_backtrace_re static pattern is a valid regex") + }) } fn xtensa_addr_pair_re() -> &'static Regex { static RE: OnceLock = OnceLock::new(); - RE.get_or_init(|| Regex::new(r"(0x[0-9a-fA-F]+):0x[0-9a-fA-F]+").unwrap()) + RE.get_or_init(|| { + Regex::new(r"(0x[0-9a-fA-F]+):0x[0-9a-fA-F]+") + .expect("fbuild-serial: xtensa_addr_pair_re static pattern is a valid regex") + }) } fn stack_pointer_re() -> &'static Regex { static RE: OnceLock = OnceLock::new(); - RE.get_or_init(|| Regex::new(r"0x(?:3[CcFf]|4[02]|50)[0-9a-fA-F]{6}").unwrap()) + RE.get_or_init(|| { + Regex::new(r"0x(?:3[CcFf]|4[02]|50)[0-9a-fA-F]{6}") + .expect("fbuild-serial: stack_pointer_re static pattern is a valid regex") + }) } fn abort_pc_re() -> &'static Regex { static RE: OnceLock = OnceLock::new(); - RE.get_or_init(|| Regex::new(r"abort\(\) was called at PC (0x[0-9a-fA-F]+)").unwrap()) + RE.get_or_init(|| { + Regex::new(r"abort\(\) was called at PC (0x[0-9a-fA-F]+)") + .expect("fbuild-serial: abort_pc_re static pattern is a valid regex") + }) } // --- CrashDecoder --- diff --git a/dylints/README.md b/dylints/README.md index 918b9abc..b3b3ff14 100644 --- a/dylints/README.md +++ b/dylints/README.md @@ -41,11 +41,16 @@ itself stays on stable 1.94.1). destructors leaks temp files, kills containment guards, and truncates in-flight HTTP/WS responses. Legacy CLI subcommand dispatchers exempted via `src/allowlist.txt`. See #826. -- **`ban_unwrap_in_daemon_handlers/`** — bans `.unwrap()` inside - `crates/fbuild-daemon/src/handlers/**/*.rs` production code (tests - exempt by filename and by `#[cfg(test)]` module walking). Locks in - PR #833's hardening; new violations would crash the daemon. See - #826. +- **`ban_unwrap_in_production/`** — bans `.unwrap()` inside production + code under `crates/fbuild-daemon/src/**/*.rs` and + `crates/fbuild-cli/src/cli/**/*.rs` (tests exempt by sibling-file + name — `tests.rs`, `*_tests.rs`, `tests_*.rs` — and by + `#[cfg(test)]` module walking). PR #833 first landed this lint for + the daemon-handler subdirectory; FastLED/fbuild#844 item 11 + widened it to all of `fbuild-daemon/src/` plus `fbuild-cli/src/cli/` + and tightened sibling-test-file detection. New violations would + crash the daemon or drop CLI users into a Rust backtrace. See #826 + and #844. - **`cli_no_build_deploy_direct_use/`** — bans `fbuild_build::*` / `fbuild_deploy::*` references in `crates/fbuild-cli/src/` outside the diagnostic-subcommand allowlist. Enforces the "thin HTTP diff --git a/dylints/ban_std_sync_mutex_in_async/src/lib.rs b/dylints/ban_std_sync_mutex_in_async/src/lib.rs index ffae07ab..be1b6608 100644 --- a/dylints/ban_std_sync_mutex_in_async/src/lib.rs +++ b/dylints/ban_std_sync_mutex_in_async/src/lib.rs @@ -129,7 +129,7 @@ impl<'tcx> LateLintPass<'tcx> for BanStdSyncMutexInAsync { } /// Walk up the HIR owner chain looking for a module annotated with -/// `#[cfg(test)]`. Mirrors `ban_unwrap_in_daemon_handlers`. +/// `#[cfg(test)]`. Mirrors `ban_unwrap_in_production`. fn owned_by_cfg_test_module(cx: &LateContext<'_>, hir_id: HirId) -> bool { let mut current = cx.tcx.hir_get_parent_item(hir_id); loop { diff --git a/dylints/ban_unwrap_in_daemon_handlers/README.md b/dylints/ban_unwrap_in_daemon_handlers/README.md deleted file mode 100644 index 2445cc81..00000000 --- a/dylints/ban_unwrap_in_daemon_handlers/README.md +++ /dev/null @@ -1,64 +0,0 @@ -# `ban_unwrap_in_daemon_handlers` - -Custom [dylint](https://github.com/trailofbits/dylint) that forbids -`.unwrap()` method calls inside files under -`crates/fbuild-daemon/src/handlers/`. Test code (`#[cfg(test)] mod -tests { ... }`, `tests.rs`, `*_tests.rs` files) is exempt — the lint -fires only on production handler code. - -## Why - -Panics inside HTTP/WebSocket handler paths crash the daemon, which -disconnects every connected client (CLI build streams, monitor -WebSockets, FastLED's `SerialMonitor`, the FastAPI bridge). Production -handlers must convert errors into structured HTTP responses or WS -error frames instead of unwrapping. - -PR #833 already fixed 10 known `.unwrap()` violations in daemon -handlers (see the websocket hardening section of that PR). This lint -locks in that state and prevents new violations from sneaking in. - -## Scope - -The lint runs only on files matching: - -``` -crates/fbuild-daemon/src/handlers/**/*.rs -``` - -Within scope, the following are exempt: - -- Files whose name matches `*tests*.rs` (e.g. `websockets_tests.rs`, - `tests_npm_cache.rs`, `tests_process.rs`, - `tests_select_runner.rs`). -- `.unwrap()` calls inside a module under a `#[cfg(test)]` attribute - (the lint walks up the owning module chain). - -Everything else (`crates/fbuild-daemon/src/{context,broker,...}.rs`, -the rest of the daemon, all other crates) is out of scope. - -## Allowlist - -This lint has no per-call allowlist file. Either the call site is in -test code (exempt automatically) or it is a real handler bug that must -be fixed. If you genuinely need an unwrap in a handler (e.g. -serializing a known-good struct that cannot fail), prefer -`.unwrap_or_default()`, `.expect("…")`, or a structured error response -— and document the invariant inline. - -## Toolchain - -Pinned to the same `nightly-2026-03-26` channel and the same -`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints -use. - -## Running locally - -See `dylints/README.md` for the full local-run recipe. CI runs all -dylints on every push/PR via `.github/workflows/dylint.yml`. - -## See also - -- Issue #826 — this lint's tracking issue -- PR #833 — original hardening that fixed the existing 10 unwrap - violations diff --git a/dylints/ban_unwrap_in_daemon_handlers/Cargo.toml b/dylints/ban_unwrap_in_production/Cargo.toml similarity index 76% rename from dylints/ban_unwrap_in_daemon_handlers/Cargo.toml rename to dylints/ban_unwrap_in_production/Cargo.toml index 853ceae5..030f55c2 100644 --- a/dylints/ban_unwrap_in_daemon_handlers/Cargo.toml +++ b/dylints/ban_unwrap_in_production/Cargo.toml @@ -1,7 +1,7 @@ -[package] -name = "ban_unwrap_in_daemon_handlers" +[package] +name = "ban_unwrap_in_production" version = "0.1.0" -description = "Ban .unwrap() inside fbuild-daemon HTTP/WS handler code" +description = "Ban .unwrap() inside fbuild production code (daemon + cli/cli/**)" edition = "2021" publish = false @@ -9,7 +9,7 @@ publish = false crate-type = ["cdylib"] [dependencies] -# Pinned to the same dylint_linting rev the other fbuild dylints use — +# Pinned to the same dylint_linting rev the other fbuild dylints use -- # same rustup channel in rust-toolchain.toml, same upstream commit. dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } @@ -17,4 +17,4 @@ dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } [package.metadata.rust-analyzer] -rustc_private = true +rustc_private = true \ No newline at end of file diff --git a/dylints/ban_unwrap_in_production/README.md b/dylints/ban_unwrap_in_production/README.md new file mode 100644 index 00000000..9806c6b7 --- /dev/null +++ b/dylints/ban_unwrap_in_production/README.md @@ -0,0 +1,78 @@ +# `ban_unwrap_in_production` + +Custom [dylint](https://github.com/trailofbits/dylint) that forbids +`.unwrap()` method calls inside fbuild *production* code: + +- `crates/fbuild-daemon/src/**/*.rs` +- `crates/fbuild-cli/src/cli/**/*.rs` + +Test code (`#[cfg(test)] mod tests { ... }`, sibling `tests.rs`, +`*_tests.rs`, `tests_*.rs` files) is exempt — the lint fires only on +production code. + +## Why + +In the daemon, panics in any code path (HTTP/WebSocket handler, broker +worker, lock manager, build dispatcher) crash the process, which +disconnects every connected client (CLI build streams, monitor +WebSockets, FastLED's `SerialMonitor`, the FastAPI bridge). Production +code must convert errors into structured HTTP responses, WS error +frames, or `Result` returns instead of panicking. + +In the CLI, panics drop the user into a Rust backtrace instead of a +clean error message and non-zero exit, which is poor UX and obscures +the real problem. + +PR #833 fixed the original daemon-handler scope (10 known violations). +FastLED/fbuild#844 item 11 widened the scope to all of +`fbuild-daemon/src/` and `fbuild-cli/src/cli/`, sweeping the remaining +production unwrap sites in a single pass. + +## Scope + +The lint runs only on files whose POSIX-normalized path contains one +of: + +``` +crates/fbuild-daemon/src/ +crates/fbuild-cli/src/cli/ +``` + +Within scope, the following are exempt: + +- Files whose name matches `tests.rs`, `*_tests.rs`, or `tests_*.rs` + (sibling test files). +- `.unwrap()` calls inside a module under a `#[cfg(test)]` attribute + (the lint walks up the owning module chain). + +Everything else (the rest of `fbuild-cli`, all other crates) is out of +scope. + +## Allowlist + +This lint has no per-call allowlist file. Either the call site is in +test code (exempt automatically) or it is a real production bug that +must be fixed. If you genuinely need an unwrap (e.g. serializing a +known-good struct that cannot fail), prefer +`.unwrap_or_else(|e| e.into_inner())` for poison-tolerant `Mutex::lock`, +`.unwrap_or_default()`, `.expect("…")` with a post-mortem-friendly +invariant message, or propagate with `?` — and document the invariant +inline. + +## Toolchain + +Pinned to the same `nightly-2026-03-26` channel and the same +`trailofbits/dylint` git rev (`4bd91ce…`) the other fbuild dylints +use. + +## Running locally + +See `dylints/README.md` for the full local-run recipe. CI runs all +dylints on every push/PR via `.github/workflows/dylint.yml`. + +## See also + +- Issue #826 — original tracking issue +- PR #833 — initial hardening of daemon handlers +- FastLED/fbuild#844 item 11 — production-scope widening + sibling + test-file detection refinement diff --git a/dylints/ban_unwrap_in_daemon_handlers/rust-toolchain.toml b/dylints/ban_unwrap_in_production/rust-toolchain.toml similarity index 100% rename from dylints/ban_unwrap_in_daemon_handlers/rust-toolchain.toml rename to dylints/ban_unwrap_in_production/rust-toolchain.toml diff --git a/dylints/ban_unwrap_in_daemon_handlers/src/allowlist.txt b/dylints/ban_unwrap_in_production/src/allowlist.txt similarity index 100% rename from dylints/ban_unwrap_in_daemon_handlers/src/allowlist.txt rename to dylints/ban_unwrap_in_production/src/allowlist.txt diff --git a/dylints/ban_unwrap_in_daemon_handlers/src/lib.rs b/dylints/ban_unwrap_in_production/src/lib.rs similarity index 55% rename from dylints/ban_unwrap_in_daemon_handlers/src/lib.rs rename to dylints/ban_unwrap_in_production/src/lib.rs index 5fd21b65..69bb613c 100644 --- a/dylints/ban_unwrap_in_daemon_handlers/src/lib.rs +++ b/dylints/ban_unwrap_in_production/src/lib.rs @@ -13,33 +13,42 @@ use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; dylint_linting::declare_late_lint! { /// ### What it does /// - /// Bans `.unwrap()` method calls inside files under - /// `crates/fbuild-daemon/src/handlers/` (any depth), with two - /// exemptions: - /// * test files (path tail matches `*tests*.rs`), and + /// Bans `.unwrap()` method calls inside fbuild *production* code: + /// * `crates/fbuild-daemon/src/**` (any depth), and + /// * `crates/fbuild-cli/src/cli/**` (any depth). + /// + /// The following are exempt: + /// * sibling test files (path tail matches `tests.rs`, + /// `*_tests.rs`, or `tests_*.rs`), and /// * `.unwrap()` calls inside a `#[cfg(test)] mod` block (the /// lint walks the owning module chain looking for the /// `cfg(test)` attribute). /// /// ### Why is this bad? /// - /// Panics in HTTP/WebSocket handler paths crash the daemon - /// process, which disconnects every connected client (CLI build - /// streams, monitor WebSockets, FastLED's `SerialMonitor`, the - /// FastAPI bridge). Handlers must convert errors into structured - /// HTTP responses or WS error frames, not panic. + /// In the daemon, panics in any code path (HTTP/WebSocket + /// handler, broker worker, lock manager, build dispatcher) crash + /// the process, which disconnects every connected client (CLI + /// build streams, monitor WebSockets, FastLED's `SerialMonitor`, + /// the FastAPI bridge). Production code must convert errors into + /// structured HTTP responses, WS error frames, or `Result` + /// returns instead of panicking. + /// + /// In the CLI, panics drop the user into a Rust backtrace + /// instead of a clean error message and non-zero exit, which is + /// a poor UX and obscures the actual problem. /// - /// PR #833 already fixed the 10 known pre-existing `.unwrap()` - /// violations in daemon handlers; this lint locks in that state - /// so new violations can't sneak in. + /// PR #833 fixed the original daemon-handler scope. FastLED/fbuild#844 + /// item 11 widened the scope to all of `fbuild-daemon/src/` and + /// `fbuild-cli/src/cli/`. /// /// ### Known problems /// /// - The lint walks owning modules looking for `#[cfg(test)]`. /// It does NOT detect `#[cfg(test)]` on individual *items* /// (e.g. `#[cfg(test)] fn helper() {}` in a non-test module). - /// Such items are rare in handler code and should be moved - /// under a proper `#[cfg(test)] mod tests` instead. + /// Such items should be moved under a proper + /// `#[cfg(test)] mod tests` instead. /// - The lint resolves the *method* DefId to `Option::unwrap` / /// `Result::unwrap`. Custom types that define their own /// `unwrap()` method are not affected. @@ -55,11 +64,12 @@ dylint_linting::declare_late_lint! { /// ``` /// /// Use instead: `.unwrap_or_default()`, `.expect("…")` with a - /// post-mortem-friendly message, or convert the error into a - /// structured response. - pub BAN_UNWRAP_IN_DAEMON_HANDLERS, + /// post-mortem-friendly message, `.unwrap_or_else(|e| e.into_inner())` + /// for poison-tolerant `Mutex::lock()`, or convert the error + /// into a structured response (`?` when the fn returns `Result`). + pub BAN_UNWRAP_IN_PRODUCTION, Deny, - "ban .unwrap() inside fbuild-daemon HTTP/WS handler code" + "ban .unwrap() inside fbuild production code (daemon + cli/cli/**)" } /// Methods we ban. Matched on the canonical DefId path returned by @@ -73,10 +83,14 @@ const BANNED_METHOD_PATHS: &[&[&str]] = &[ &["std", "result", "Result", "unwrap"], ]; -/// Scope: any file path matching this prefix is in scope. -const HANDLERS_DIR: &str = "crates/fbuild-daemon/src/handlers/"; +/// In-scope path prefixes. A file is in scope if its (normalized, +/// forward-slash) path contains any of these substrings. +const IN_SCOPE_PREFIXES: &[&str] = &[ + "crates/fbuild-daemon/src/", + "crates/fbuild-cli/src/cli/", +]; -impl<'tcx> LateLintPass<'tcx> for BanUnwrapInDaemonHandlers { +impl<'tcx> LateLintPass<'tcx> for BanUnwrapInProduction { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { let filename = source_filename(cx, expr.span); let normalized = normalize_slashes(&filename); @@ -154,16 +168,19 @@ fn attr_is_cfg_test(attr: &rustc_hir::Attribute) -> bool { fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { cx.opt_span_lint( - BAN_UNWRAP_IN_DAEMON_HANDLERS, + BAN_UNWRAP_IN_PRODUCTION, Some(span), DiagDecorator(|diag| { diag.primary_message( - "`.unwrap()` inside an fbuild-daemon handler can crash the daemon and \ - disconnect every connected client. Convert the error into a structured \ - response (HTTP 500 / WS error frame) or use `.unwrap_or_default()` / \ - `.expect(\"\")`. Tests are exempt — either \ - move the call into a `#[cfg(test)] mod tests { ... }` block or rename \ - the file to `*tests*.rs`.", + "`.unwrap()` inside fbuild production code (fbuild-daemon/src/** or \ + fbuild-cli/src/cli/**) can crash the daemon / drop the CLI user into \ + a Rust backtrace. Convert the error into a structured response \ + (HTTP 500 / WS error frame), propagate it with `?` if the fn returns \ + `Result`, use `.unwrap_or_else(|e| e.into_inner())` for poison-tolerant \ + `Mutex::lock()`, or use `.expect(\"\")` \ + with an actionable invariant message. Tests are exempt — either move \ + the call into a `#[cfg(test)] mod tests { ... }` block or put it in \ + a sibling `tests.rs` / `*_tests.rs` / `tests_*.rs` file.", ); }), ); @@ -187,19 +204,28 @@ fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { } fn in_scope(normalized: &str) -> bool { - normalized.contains(HANDLERS_DIR) + IN_SCOPE_PREFIXES + .iter() + .any(|prefix| normalized.contains(prefix)) } +/// A "sibling test file" is one whose base name marks the file itself +/// as test code (not production), matching: +/// * `tests.rs` +/// * `*_tests.rs` (e.g. `websockets_tests.rs`) +/// * `tests_*.rs` (e.g. `tests_npm_cache.rs`, `tests_process.rs`) fn is_test_file(normalized: &str) -> bool { let Some(name) = normalized.rsplit('/').next() else { return false; }; - // Match files whose base name contains "tests" (e.g. - // `websockets_tests.rs`, `tests_npm_cache.rs`, - // `tests_process.rs`, `tests_select_runner.rs`, `tests_outcome.rs`, - // `tests.rs`). A plain `tests` substring is enough — the handler - // tree doesn't use `tests` as a prefix for non-test code. - name.contains("tests") && name.ends_with(".rs") + if !name.ends_with(".rs") { + return false; + } + if name == "tests.rs" { + return true; + } + let stem = &name[..name.len() - 3]; // strip ".rs" + stem.ends_with("_tests") || stem.starts_with("tests_") } fn normalize_slashes(path: &str) -> String { @@ -226,7 +252,7 @@ mod tests { use super::*; #[test] - fn handler_paths_are_in_scope() { + fn daemon_paths_are_in_scope() { assert!(in_scope( "crates/fbuild-daemon/src/handlers/operations/build.rs" )); @@ -234,31 +260,70 @@ mod tests { assert!(in_scope( "/anywhere/crates/fbuild-daemon/src/handlers/emulator/qemu_deploy.rs" )); + // Widened scope: all of fbuild-daemon/src/, not just handlers/. + assert!(in_scope("crates/fbuild-daemon/src/context.rs")); + assert!(in_scope("crates/fbuild-daemon/src/main.rs")); + assert!(in_scope("crates/fbuild-daemon/src/broker/session.rs")); + } + + #[test] + fn cli_cli_paths_are_in_scope() { + assert!(in_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(in_scope("crates/fbuild-cli/src/cli/deploy.rs")); + assert!(in_scope( + "/anywhere/crates/fbuild-cli/src/cli/clang_tools.rs" + )); + } + + #[test] + fn cli_non_cli_paths_are_out_of_scope() { + // Only crates/fbuild-cli/src/cli/** is in scope, not the rest of fbuild-cli. + assert!(!in_scope("crates/fbuild-cli/src/main.rs")); + assert!(!in_scope("crates/fbuild-cli/src/lib_select.rs")); + assert!(!in_scope("crates/fbuild-cli/src/mcp/server.rs")); } #[test] - fn non_handler_paths_are_out_of_scope() { - assert!(!in_scope("crates/fbuild-daemon/src/context.rs")); - assert!(!in_scope("crates/fbuild-daemon/src/main.rs")); - assert!(!in_scope("crates/fbuild-cli/src/cli/build.rs")); + fn other_crates_are_out_of_scope() { + assert!(!in_scope("crates/fbuild-core/src/lib.rs")); + assert!(!in_scope("crates/fbuild-build/src/compiler.rs")); + assert!(!in_scope("crates/fbuild-serial/src/crash_decoder.rs")); } #[test] - fn test_files_are_exempt_by_name() { + fn sibling_test_files_are_exempt() { + // Plain `tests.rs` assert!(is_test_file( - "crates/fbuild-daemon/src/handlers/websockets_tests.rs" + "crates/fbuild-daemon/src/handlers/operations/tests.rs" )); + // `*_tests.rs` assert!(is_test_file( - "crates/fbuild-daemon/src/handlers/operations/tests.rs" + "crates/fbuild-daemon/src/handlers/websockets_tests.rs" )); + // `tests_*.rs` assert!(is_test_file( "crates/fbuild-daemon/src/handlers/emulator/tests_npm_cache.rs" )); + assert!(is_test_file( + "crates/fbuild-cli/src/cli/tests_select_runner.rs" + )); + } + + #[test] + fn production_files_are_not_marked_as_test() { + // Per FastLED/fbuild#844 item 11: the old substring-based test detection + // accidentally exempted any file with "tests" in its name. The new + // detection requires the specific suffixes/prefixes above. assert!(!is_test_file( "crates/fbuild-daemon/src/handlers/operations/build.rs" )); assert!(!is_test_file( "crates/fbuild-daemon/src/handlers/websockets.rs" )); + // A production file that merely contains "tests" but doesn't match + // the patterns must NOT be exempt. + assert!(!is_test_file( + "crates/fbuild-daemon/src/run_tests_pipeline.rs" + )); } }