From 01892185ff04bfa7707d79c237f9dfcaa4ead949 Mon Sep 17 00:00:00 2001 From: "fabro-sh-0530[bot]" <281434857+fabro-sh-0530[bot]@users.noreply.github.com> Date: Tue, 26 May 2026 17:46:39 -0400 Subject: [PATCH] =?UTF-8?q?Replace=20bare=20unwrap()=20with=20documented?= =?UTF-8?q?=20expect()=20across=20production=20runtim=E2=80=A6=20(#415)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit and remediation pass enforcing the project's no-panic-in-production policy. Every `unwrap()` on a mutex/RwLock in reachable runtime code is replaced with `expect()` carrying a message that explains *why* the lock cannot be poisoned (no code panics while holding it). Bare `unreachable!()` and `panic!()` calls are updated with messages that name the invariant being asserted. One genuine bug is fixed in the process. ## What changed **`unwrap()` → `expect()` on locks** (`fabro-core`, `fabro-oauth`, `fabro-util`, `fabro-workflow/*`, `fabro-server`): Every `Mutex`/`RwLock` `.unwrap()` in production paths now carries the standard justification pattern: `" mutex/RwLock should not be poisoned: no code panics while holding this lock"`. **`unreachable!()` and `panic!()` message quality**: Bare `unreachable!()` calls in `subagent.rs`, `wait.rs`, `condition.rs`, `event/convert.rs`, and `server.rs` now name the structural invariant (e.g. "outer match arm already verified…"). The `panic!` in `tools.rs` now includes the offending name and the expected format, making it actionable. **`sha_newtype` / `short_sha_newtype` in `run_files.rs` — actual bug fix**: These helpers previously called `unwrap_or_else(|e| panic!(…))` on git output, meaning a malformed SHA from a real git subprocess would panic in a request handler. They now return `Result` and propagate errors to callers, which in turn propagate with `?`. This is the only change that alters observable behavior under failure. **Demo-only panics in `fabro-server/src/demo/mod.rs`**: Panic messages updated to clarify that these paths operate on hardcoded compile-time constants, so the panic is a programming-error guard rather than a runtime failure guard. ## Design note The lock-poisoning `expect` messages all follow a single template so reviewers can quickly verify the claim: if you ever add code that can panic inside a lock guard scope, the message becomes a lie and that must be caught in review. The uniformity is intentional. ### Fabro Details
Ran 0 stages in 64m 54s for $14.28 | Stage | Duration | Cost | Retries | |---|---|---|---| | **Total** | **64m 54s** | **$14.28** | **0** |
Ran Goal.fabro (4 nodes and 5 edges) ```dot digraph Goal { graph [ goal="Complete the user-provided goal", rankdir=LR, max_node_visits=30 ] start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] work [ label="Work", thread_id="goal", fidelity="full", max_visits=12, prompt="@prompts/continue.md" ] audit [ label="Completion Audit", thread_id="goal", fidelity="full", goal_gate=true, retry_target="work", output_schema="routing", output_retries=2, max_visits=12, prompt="@prompts/audit.md" ] start -> work -> audit audit -> exit [label="Done", condition="outcome=succeeded"] audit -> work [label="Continue", condition="outcome=failed || preferred_label=Continue"] audit -> work [label="No clear verdict"] } ```
⚒️ Generated with [Fabro](https://fabro.sh) --------- Co-authored-by: Fabro Co-authored-by: Bryan Helmkamp --- lib/crates/fabro-agent/src/subagent.rs | 2 +- lib/crates/fabro-cli/src/commands/install.rs | 17 ++++- lib/crates/fabro-cli/src/commands/run/wait.rs | 4 +- lib/crates/fabro-core/src/context.rs | 20 ++++-- .../src/providers/openai_compatible.rs | 4 +- lib/crates/fabro-llm/src/tools.rs | 18 +++-- lib/crates/fabro-oauth/src/lib.rs | 65 +++++++++++++++---- lib/crates/fabro-server/src/demo/mod.rs | 13 ++-- lib/crates/fabro-server/src/run_files.rs | 42 +++++++----- lib/crates/fabro-server/src/server.rs | 4 +- lib/crates/fabro-util/src/warnings.rs | 3 +- lib/crates/fabro-workflow/src/condition.rs | 2 +- .../fabro-workflow/src/event/convert.rs | 5 +- .../fabro-workflow/src/handler/llm/api.rs | 30 +++++++-- .../fabro-workflow/src/lifecycle/artifact.rs | 26 ++++++-- .../src/lifecycle/circuit_breaker.rs | 22 +++++-- .../fabro-workflow/src/lifecycle/event.rs | 11 +++- .../fabro-workflow/src/lifecycle/fidelity.rs | 22 +++++-- .../fabro-workflow/src/lifecycle/git.rs | 21 ++++-- .../fabro-workflow/src/lifecycle/mod.rs | 7 +- .../fabro-workflow/src/operations/start.rs | 16 +++-- lib/crates/fabro-workflow/src/services.rs | 11 +++- 22 files changed, 274 insertions(+), 91 deletions(-) diff --git a/lib/crates/fabro-agent/src/subagent.rs b/lib/crates/fabro-agent/src/subagent.rs index 0c68694c4..52cec77c6 100644 --- a/lib/crates/fabro-agent/src/subagent.rs +++ b/lib/crates/fabro-agent/src/subagent.rs @@ -234,7 +234,7 @@ impl SubAgentManager { match &agent.status { SubAgentStatus::Finished(result) => result.clone(), - _ => unreachable!(), + _ => unreachable!("agent status was just assigned to Finished on the line above"), } } diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 9d0f635f5..d956aa170 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -1030,10 +1030,23 @@ async fn setup_github_app( .route( "/callback", get(move |Query(params): Query| async move { - if let Some(tx) = code_tx.lock().unwrap().take() { + if let Some(tx) = code_tx + .lock() + .expect( + "code_tx mutex is never poisoned: no code panics while holding this lock", + ) + .take() + { let _ = tx.send(params.code); } - if let Some(tx) = shutdown_tx.lock().unwrap().take() { + if let Some(tx) = shutdown_tx + .lock() + .expect( + "shutdown_tx mutex is never poisoned: no code panics while holding this \ + lock", + ) + .take() + { let _ = tx.send(()); } Html(r#" diff --git a/lib/crates/fabro-cli/src/commands/run/wait.rs b/lib/crates/fabro-cli/src/commands/run/wait.rs index 6ef61c4ee..3e2017c36 100644 --- a/lib/crates/fabro-cli/src/commands/run/wait.rs +++ b/lib/crates/fabro-cli/src/commands/run/wait.rs @@ -107,7 +107,9 @@ fn print_human_output( RunStatus::Failed { .. } => (&styles.bold_red, "Failed"), RunStatus::Dead => (&styles.bold_red, "Dead"), // Poll loop only breaks on is_terminal() which is the three arms above. - _ => unreachable!(), + _ => unreachable!( + "print_human_output is only called after is_terminal(), which returns true only for Succeeded, Failed, and Dead" + ), }; let status_display = style.apply_to(label); diff --git a/lib/crates/fabro-core/src/context.rs b/lib/crates/fabro-core/src/context.rs index 01761c2d9..5563686ec 100644 --- a/lib/crates/fabro-core/src/context.rs +++ b/lib/crates/fabro-core/src/context.rs @@ -20,11 +20,18 @@ impl Context { } pub fn set(&self, key: impl Into, value: Value) { - self.values.write().unwrap().insert(key.into(), value); + self.values + .write() + .expect("context RwLock should not be poisoned: no code panics while holding this lock") + .insert(key.into(), value); } pub fn get(&self, key: &str) -> Option { - self.values.read().unwrap().get(key).cloned() + self.values + .read() + .expect("context RwLock should not be poisoned: no code panics while holding this lock") + .get(key) + .cloned() } pub fn get_string(&self, key: &str, default: &str) -> String { @@ -34,14 +41,19 @@ impl Context { } pub fn apply_updates(&self, updates: &HashMap) { - let mut values = self.values.write().unwrap(); + let mut values = self.values.write().expect( + "context RwLock should not be poisoned: no code panics while holding this lock", + ); for (k, v) in updates { values.insert(k.clone(), v.clone()); } } pub fn snapshot(&self) -> HashMap { - self.values.read().unwrap().clone() + self.values + .read() + .expect("context RwLock should not be poisoned: no code panics while holding this lock") + .clone() } /// Deep copy for parallel branch isolation. diff --git a/lib/crates/fabro-llm/src/providers/openai_compatible.rs b/lib/crates/fabro-llm/src/providers/openai_compatible.rs index 4aeb94768..b08977b41 100644 --- a/lib/crates/fabro-llm/src/providers/openai_compatible.rs +++ b/lib/crates/fabro-llm/src/providers/openai_compatible.rs @@ -301,7 +301,9 @@ fn translate_messages(messages: &[Message]) -> Vec { Role::System | Role::Developer => "system", Role::User => "user", Role::Assistant => "assistant", - Role::Tool => unreachable!(), + Role::Tool => unreachable!( + "Role::Tool is handled in the early-return branch above this match" + ), }; let mut tool_calls: Vec = Vec::new(); diff --git a/lib/crates/fabro-llm/src/tools.rs b/lib/crates/fabro-llm/src/tools.rs index 6f64faed9..589488115 100644 --- a/lib/crates/fabro-llm/src/tools.rs +++ b/lib/crates/fabro-llm/src/tools.rs @@ -39,10 +39,15 @@ impl Tool { /// # Panics /// /// Panics if the tool name is invalid (see [`validate_tool_name`]). + /// Tool names are always hardcoded string literals in this codebase; this + /// guards against programming errors where a constant would fail + /// validation. #[must_use] pub fn passive(name: &str, description: &str, parameters: serde_json::Value) -> Self { if let Err(e) = validate_tool_name(name) { - panic!("Invalid tool name: {e}"); + panic!( + "tool name `{name}` must be a valid identifier ([a-zA-Z][a-zA-Z0-9_]*, ≤64 chars): {e}" + ); } Self { definition: ToolDefinition { @@ -59,6 +64,9 @@ impl Tool { /// # Panics /// /// Panics if the tool name is invalid (see [`validate_tool_name`]). + /// Tool names are always hardcoded string literals in this codebase; this + /// guards against programming errors where a constant would fail + /// validation. pub fn active( name: &str, description: &str, @@ -70,7 +78,9 @@ impl Tool { Fut: Future> + Send + 'static, { if let Err(e) = validate_tool_name(name) { - panic!("Invalid tool name: {e}"); + panic!( + "tool name `{name}` must be a valid identifier ([a-zA-Z][a-zA-Z0-9_]*, ≤64 chars): {e}" + ); } Self { definition: ToolDefinition { @@ -319,7 +329,7 @@ mod tests { } #[test] - #[should_panic(expected = "Invalid tool name")] + #[should_panic(expected = "must be a valid identifier")] fn passive_tool_panics_on_invalid_name() { let _ = Tool::passive( "1invalid", @@ -329,7 +339,7 @@ mod tests { } #[test] - #[should_panic(expected = "Invalid tool name")] + #[should_panic(expected = "must be a valid identifier")] fn active_tool_panics_on_invalid_name() { Tool::active( "my-tool", diff --git a/lib/crates/fabro-oauth/src/lib.rs b/lib/crates/fabro-oauth/src/lib.rs index 9d65f0c1b..3c453d049 100644 --- a/lib/crates/fabro-oauth/src/lib.rs +++ b/lib/crates/fabro-oauth/src/lib.rs @@ -293,7 +293,10 @@ impl CallbackHandle { } pub fn shutdown(&self) { - if let Some(tx) = self.shutdown_tx.lock().unwrap().take() { + if let Some(tx) = self.shutdown_tx.lock() + .expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } } @@ -486,10 +489,16 @@ pub async fn start_callback_server( let desc = params .error_description .unwrap_or_else(|| error.clone()); - if let Some(tx) = code_tx.lock().unwrap().take() { + if let Some(tx) = code_tx.lock() + .expect("oauth code_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(Err(desc.clone())); } - if let Some(tx) = shutdown_tx.lock().unwrap().take() { + if let Some(tx) = shutdown_tx.lock() + .expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } return ( @@ -499,10 +508,16 @@ pub async fn start_callback_server( } let Some(code) = params.code else { - if let Some(tx) = code_tx.lock().unwrap().take() { + if let Some(tx) = code_tx.lock() + .expect("oauth code_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(Err("No authorization code received".to_string())); } - if let Some(tx) = shutdown_tx.lock().unwrap().take() { + if let Some(tx) = shutdown_tx.lock() + .expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } return ( @@ -513,10 +528,16 @@ pub async fn start_callback_server( ); }; - if let Some(tx) = code_tx.lock().unwrap().take() { + if let Some(tx) = code_tx.lock() + .expect("oauth code_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(Ok(code)); } - if let Some(tx) = shutdown_tx.lock().unwrap().take() { + if let Some(tx) = shutdown_tx.lock() + .expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } (StatusCode::OK, Html(callback_success_page())) @@ -576,13 +597,19 @@ pub async fn start_callback_server_with_errors( let error_description = params .error_description .unwrap_or_else(|| error_code.clone()); - if let Some(tx) = callback_tx.lock().unwrap().take() { + if let Some(tx) = callback_tx.lock() + .expect("oauth callback_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(Err(CallbackFailure { error_code: error_code.clone(), error_description: error_description.clone(), })); } - if let Some(tx) = route_shutdown_tx.lock().unwrap().take() { + if let Some(tx) = route_shutdown_tx.lock() + .expect("oauth route_shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } return ( @@ -592,13 +619,19 @@ pub async fn start_callback_server_with_errors( } let Some(code) = params.code else { - if let Some(tx) = callback_tx.lock().unwrap().take() { + if let Some(tx) = callback_tx.lock() + .expect("oauth callback_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(Err(CallbackFailure { error_code: "invalid_request".to_string(), error_description: "No authorization code received".to_string(), })); } - if let Some(tx) = route_shutdown_tx.lock().unwrap().take() { + if let Some(tx) = route_shutdown_tx.lock() + .expect("oauth route_shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } return ( @@ -609,10 +642,16 @@ pub async fn start_callback_server_with_errors( ); }; - if let Some(tx) = callback_tx.lock().unwrap().take() { + if let Some(tx) = callback_tx.lock() + .expect("oauth callback_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(Ok(CallbackSuccess { code })); } - if let Some(tx) = route_shutdown_tx.lock().unwrap().take() { + if let Some(tx) = route_shutdown_tx.lock() + .expect("oauth route_shutdown_tx mutex should not be poisoned: no code panics while holding this lock") + .take() + { let _ = tx.send(()); } (StatusCode::OK, Html(callback_success_page())) diff --git a/lib/crates/fabro-server/src/demo/mod.rs b/lib/crates/fabro-server/src/demo/mod.rs index e2c9b1885..ffd4fbed9 100644 --- a/lib/crates/fabro-server/src/demo/mod.rs +++ b/lib/crates/fabro-server/src/demo/mod.rs @@ -243,7 +243,9 @@ where T: TryFrom, >::Error: std::fmt::Display, { - T::try_from(sha.to_string()).unwrap_or_else(|err| panic!("invalid demo SHA `{sha}`: {err}")) + T::try_from(sha.to_string()).unwrap_or_else(|err| { + panic!("demo SHA `{sha}` is a hardcoded constant and must match the hex pattern: {err}") + }) } fn short_sha_newtype(sha: &str) -> T @@ -252,8 +254,11 @@ where >::Error: std::fmt::Display, { let short = sha.chars().take(7).collect::(); - T::try_from(short.clone()) - .unwrap_or_else(|err| panic!("invalid demo short SHA `{short}`: {err}")) + T::try_from(short.clone()).unwrap_or_else(|err| { + panic!( + "demo short SHA `{short}` is a hardcoded constant and must match the hex pattern: {err}" + ) + }) } fn demo_run_files() -> PaginatedRunFileList { @@ -1128,7 +1133,7 @@ mod runs { labels: labels(entries), lifecycle: RunLifecycle { status: parse_run_status(status, status_reason) - .unwrap_or_else(|| panic!("invalid demo run status: {status}")), + .unwrap_or_else(|| panic!("demo run status `{status}` is a hardcoded constant and must be a valid RunStatus variant")), approval: None, pending_control, queue_position: None, diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index 8ef410972..53657282a 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -345,8 +345,8 @@ async fn materialize_run_commits( data: commits, meta: RunCommitsMeta { source: RunCommitsMetaSource::Sandbox, - base_sha: sha_newtype::(&base_sha), - head_sha: sha_newtype::(&head_sha), + base_sha: sha_newtype::(&base_sha)?, + head_sha: sha_newtype::(&head_sha)?, limit: NonZeroU64::new(limit).expect("commit limit is non-zero"), total_returned, truncated, @@ -410,15 +410,17 @@ fn parse_git_log_commit(record: &str) -> std::result::Result(parent), - short_sha: short_sha_newtype::(parent), + .map(|parent| { + Ok(RunCommitParent { + sha: sha_newtype::(parent)?, + short_sha: short_sha_newtype::(parent)?, + }) }) - .collect(); + .collect::, ApiError>>()?; Ok(RunCommit { - sha: sha_newtype::(sha), - short_sha: short_sha_newtype::(sha), + sha: sha_newtype::(sha)?, + short_sha: short_sha_newtype::(sha)?, parents, author: RunCommitPerson { name: author_name.to_string(), @@ -434,7 +436,11 @@ fn parse_git_log_commit(record: &str) -> std::result::Result(tree_sha)), + tree_sha: if tree_sha.is_empty() { + None + } else { + Some(sha_newtype::(tree_sha)?) + }, }) } @@ -472,23 +478,29 @@ fn parse_git_date(value: &str) -> Option> { .map(|d| d.with_timezone(&chrono::Utc)) } -fn sha_newtype(sha: &str) -> T +fn sha_newtype(sha: &str) -> std::result::Result where T: TryFrom, >::Error: std::fmt::Display, { - T::try_from(sha.to_string()) - .unwrap_or_else(|err| panic!("invalid generated SHA `{sha}`: {err}")) + T::try_from(sha.to_string()).map_err(|err| { + ApiError::bad_request(format!( + "git returned a SHA that did not match expected hex pattern: `{sha}`: {err}" + )) + }) } -fn short_sha_newtype(sha: &str) -> T +fn short_sha_newtype(sha: &str) -> std::result::Result where T: TryFrom, >::Error: std::fmt::Display, { let short = sha.chars().take(7).collect::(); - T::try_from(short.clone()) - .unwrap_or_else(|err| panic!("invalid generated short SHA `{short}`: {err}")) + T::try_from(short.clone()).map_err(|err| { + ApiError::bad_request(format!( + "git returned a short SHA that did not match expected hex pattern: `{short}`: {err}" + )) + }) } /// Materialize the response for `GET /runs/{id}/files`. Prefers the live diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 96a1dfcf6..dfd192655 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -3079,7 +3079,9 @@ fn update_live_run_from_event(state: &AppState, run_id: RunId, event: &RunEvent) EventBody::RunRemoving(_) => managed_run.status = RunStatus::Removing, EventBody::RunCompleted(_) => { let EventBody::RunCompleted(props) = &event.body else { - unreachable!(); + unreachable!( + "outer match arm already verified event.body is EventBody::RunCompleted" + ) }; managed_run.status = RunStatus::Succeeded { reason: props.reason, diff --git a/lib/crates/fabro-util/src/warnings.rs b/lib/crates/fabro-util/src/warnings.rs index 72ff7a0c4..9b44ee216 100644 --- a/lib/crates/fabro-util/src/warnings.rs +++ b/lib/crates/fabro-util/src/warnings.rs @@ -20,7 +20,8 @@ macro_rules! warn_user { macro_rules! warn_user_once { ($($arg:tt)*) => {{ let message = format!($($arg)*); - let mut set = $crate::WARNINGS.lock().unwrap(); + let mut set = $crate::WARNINGS.lock() + .expect("WARNINGS mutex should not be poisoned: no code panics while holding this lock"); if set.insert(message.clone()) { drop(set); $crate::warn_user!("{message}"); diff --git a/lib/crates/fabro-workflow/src/condition.rs b/lib/crates/fabro-workflow/src/condition.rs index 060d40891..8ef707229 100644 --- a/lib/crates/fabro-workflow/src/condition.rs +++ b/lib/crates/fabro-workflow/src/condition.rs @@ -114,7 +114,7 @@ fn eval_clause(clause: &Clause, outcome: &Outcome, context: &Context) -> bool { Op::Lt => lhs < rhs, Op::Gte => lhs >= rhs, Op::Lte => lhs <= rhs, - _ => unreachable!(), + _ => unreachable!("outer match arm already restricts to Gt, Lt, Gte, and Lte"), } } Op::Contains => { diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index 86a0fef41..684af5164 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -834,8 +834,9 @@ fn event_body_from_event(event: &Event) -> EventBody { | AgentEvent::ReasoningDelta { .. } | AgentEvent::ToolCallOutputDelta { .. } | AgentEvent::SessionStarted { .. } - | AgentEvent::SessionEnded => panic!( - "agent event should not be converted through the stage-scoped Event::Agent wrapper" + | AgentEvent::SessionEnded => unreachable!( + "streaming noise and session lifecycle events are filtered out before wrapping in \ + Event::Agent; if this is reached, the emitter has a routing bug" ), }, Event::SubgraphStarted { start_node, .. } => { diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index fc9c6998c..e00a7c255 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -482,14 +482,20 @@ fn track_file_event(event: &AgentEvent, state: &mut FileTracking) { fn file_tracking_snapshot( file_tracking: &Arc>, ) -> (Vec, Option) { - let state = file_tracking.lock().unwrap(); + let state = file_tracking + .lock() + .expect("file_tracking mutex is never poisoned: no code panics while holding this lock"); let mut files: Vec = state.touched.iter().cloned().collect(); files.sort(); (files, state.last.clone()) } fn last_touched_file(file_tracking: &Arc>) -> Option { - file_tracking.lock().unwrap().last.clone() + file_tracking + .lock() + .expect("file_tracking mutex is never poisoned: no code panics while holding this lock") + .last + .clone() } fn last_assistant_response(session: &Session) -> String { @@ -540,7 +546,12 @@ fn spawn_event_forwarder( emitter.touch(); // Track file changes from tool calls (including sub-agent events) - track_file_event(&event.event, &mut file_tracking.lock().unwrap()); + track_file_event( + &event.event, + &mut file_tracking.lock().expect( + "file_tracking mutex is never poisoned: no code panics while holding this lock", + ), + ); // Forward non-streaming agent events to pipeline if !event.event.is_streaming_noise() @@ -888,7 +899,7 @@ impl AgentApiBackend { let sessions: Vec = self .sessions .lock() - .unwrap() + .expect("sessions mutex is never poisoned: no code panics while holding this lock") .drain() .map(|(_, s)| s) .collect(); @@ -1152,7 +1163,11 @@ impl CodergenBackend for AgentApiBackend { return Err(Error::Cancelled); } let (mut session, is_reused) = if let Some(ref key) = reuse_key { - let existing = self.sessions.lock().unwrap().remove(key); + let existing = self + .sessions + .lock() + .expect("sessions mutex is never poisoned: no code panics while holding this lock") + .remove(key); if let Some(s) = existing { (s, true) } else { @@ -1521,7 +1536,10 @@ impl CodergenBackend for AgentApiBackend { // the cached session is not left wired to this run's cancel token. if let Some(key) = reuse_key { bridge.abort(); - self.sessions.lock().unwrap().insert(key, session); + self.sessions + .lock() + .expect("sessions mutex is never poisoned: no code panics while holding this lock") + .insert(key, session); } else { let session_id = session.id().to_string(); if session.close() { diff --git a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs index 072a0f76c..6d8b8de75 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs @@ -73,7 +73,9 @@ impl ArtifactLifecycle { #[async_trait] impl RunLifecycle for ArtifactLifecycle { async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> { - *self.attempt_start_epoch.lock().unwrap() = None; + *self.attempt_start_epoch.lock().expect( + "artifact mutex should not be poisoned: no code panics while holding this lock", + ) = None; let ledger = self .rebuild_captured_artifact_ledger() .await @@ -83,7 +85,9 @@ impl RunLifecycle for ArtifactLifecycle { "failed to rebuild captured artifact ledger: {rendered}" )) })?; - *self.captured_artifacts.lock().unwrap() = ledger; + *self.captured_artifacts.lock().expect( + "artifact mutex should not be poisoned: no code panics while holding this lock", + ) = ledger; Ok(()) } @@ -96,7 +100,9 @@ impl RunLifecycle for ArtifactLifecycle { let epoch = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map_or(0.0, |d| d.as_secs() as f64); - *self.attempt_start_epoch.lock().unwrap() = Some(epoch); + *self.attempt_start_epoch.lock().expect( + "artifact mutex should not be poisoned: no code panics while holding this lock", + ) = Some(epoch); Ok(NodeDecision::Continue) } @@ -108,7 +114,11 @@ impl RunLifecycle for ArtifactLifecycle { if self.artifact_globs.is_empty() { return Ok(()); } - let epoch = self.attempt_start_epoch.lock().unwrap().unwrap_or(0.0); + let epoch = self + .attempt_start_epoch + .lock() + .expect("artifact mutex should not be poisoned: no code panics while holding this lock") + .unwrap_or(0.0); let node_id = ctx.node.id(); let visit = stage_visit(state, node_id); let node_slug = if visit <= 1 { @@ -256,7 +266,9 @@ impl ArtifactLifecycle { } fn new_captured_assets(&self, artifacts: &[ArtifactUpload]) -> Vec { - let ledger = self.captured_artifacts.lock().unwrap(); + let ledger = self.captured_artifacts.lock().expect( + "artifact mutex should not be poisoned: no code panics while holding this lock", + ); artifacts .iter() .filter(|artifact| !ledger.contains(&artifact_identity(artifact))) @@ -265,7 +277,9 @@ impl ArtifactLifecycle { } fn record_captured_assets(&self, artifacts: &[ArtifactUpload]) { - let mut ledger = self.captured_artifacts.lock().unwrap(); + let mut ledger = self.captured_artifacts.lock().expect( + "artifact mutex should not be poisoned: no code panics while holding this lock", + ); for artifact in artifacts { ledger.insert(artifact_identity(artifact)); } diff --git a/lib/crates/fabro-workflow/src/lifecycle/circuit_breaker.rs b/lib/crates/fabro-workflow/src/lifecycle/circuit_breaker.rs index da94d9d10..0b29fee43 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/circuit_breaker.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/circuit_breaker.rs @@ -37,8 +37,12 @@ impl CircuitBreakerLifecycle { loop_sigs: HashMap, restart_sigs: HashMap, ) { - *self.loop_failure_signatures.lock().unwrap() = loop_sigs; - *self.restart_failure_signatures.lock().unwrap() = restart_sigs; + *self.loop_failure_signatures.lock().expect( + "circuit breaker mutex should not be poisoned: no code panics while holding this lock", + ) = loop_sigs; + *self.restart_failure_signatures.lock().expect( + "circuit breaker mutex should not be poisoned: no code panics while holding this lock", + ) = restart_sigs; } /// Snapshot current state for checkpoint building. @@ -48,8 +52,12 @@ impl CircuitBreakerLifecycle { HashMap, HashMap, ) { - let loop_sigs = self.loop_failure_signatures.lock().unwrap().clone(); - let restart_sigs = self.restart_failure_signatures.lock().unwrap().clone(); + let loop_sigs = self.loop_failure_signatures.lock() + .expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock") + .clone(); + let restart_sigs = self.restart_failure_signatures.lock() + .expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock") + .clone(); (loop_sigs, restart_sigs) } } @@ -83,7 +91,8 @@ impl RunLifecycle for CircuitBreakerLifecycle { outcome.failure.as_ref().map(|f| f.message.as_str()), ); if fc.is_signature_tracked() { - let mut sigs = self.loop_failure_signatures.lock().unwrap(); + let mut sigs = self.loop_failure_signatures.lock() + .expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock"); let count = sigs.entry(sig.clone()).or_insert(0); *count += 1; let limit = self.loop_restart_signature_limit; @@ -133,7 +142,8 @@ impl RunLifecycle for CircuitBreakerLifecycle { Some(failure.message.as_str()), ); if failure.category.is_signature_tracked() { - let mut sigs = self.restart_failure_signatures.lock().unwrap(); + let mut sigs = self.restart_failure_signatures.lock() + .expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock"); let count = sigs.entry(sig.clone()).or_insert(0); *count += 1; let limit = self.loop_restart_signature_limit; diff --git a/lib/crates/fabro-workflow/src/lifecycle/event.rs b/lib/crates/fabro-workflow/src/lifecycle/event.rs index 3226abac1..5828dc86b 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/event.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/event.rs @@ -111,7 +111,8 @@ impl RunLifecycle for EventLifecycle { async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> { // If restarted_from is Some, emit LoopRestart and clear it { - let mut restarted = self.restarted_from.lock().unwrap(); + let mut restarted = self.restarted_from.lock() + .expect("event lifecycle mutex should not be poisoned: no code panics while holding this lock"); if let Some((from_node, to_node)) = restarted.take() { self.emitter .emit(&Event::LoopRestart { from_node, to_node }); @@ -119,7 +120,9 @@ impl RunLifecycle for EventLifecycle { } // Reset run_start for duration measurement - *self.run_start.lock().unwrap() = Instant::now(); + *self.run_start.lock().expect( + "event lifecycle mutex should not be poisoned: no code panics while holding this lock", + ) = Instant::now(); // Emit RunStarted self.emitter.emit(&Event::WorkflowRunStarted { @@ -377,7 +380,9 @@ impl RunLifecycle for EventLifecycle { let status = result.outcome.status.to_string(); // Read git checkpoint result (set by GitLifecycle) - let git_result = self.checkpoint_git_result.lock().unwrap().clone(); + let git_result = self.checkpoint_git_result.lock() + .expect("event lifecycle mutex should not be poisoned: no code panics while holding this lock") + .clone(); let git_sha = git_result.as_ref().and_then(|r| r.commit_sha.clone()); let diff = git_result.as_ref().and_then(|r| r.diff.clone()); diff --git a/lib/crates/fabro-workflow/src/lifecycle/fidelity.rs b/lib/crates/fabro-workflow/src/lifecycle/fidelity.rs index 32b170ad2..3f45c8142 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/fidelity.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/fidelity.rs @@ -57,7 +57,9 @@ impl FidelityLifecycle { } pub(crate) fn set_degrade_fidelity_on_resume(&self, flag: bool) { - *self.degrade_fidelity_on_resume.lock().unwrap() = flag; + *self.degrade_fidelity_on_resume.lock().expect( + "fidelity mutex should not be poisoned: no code panics while holding this lock", + ) = flag; } } @@ -65,7 +67,9 @@ impl FidelityLifecycle { impl RunLifecycle for FidelityLifecycle { async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> { // Clear incoming edge data (restart target must not inherit pre-restart edge) - *self.incoming_edge_data.lock().unwrap() = None; + *self.incoming_edge_data.lock().expect( + "fidelity mutex should not be poisoned: no code panics while holding this lock", + ) = None; Ok(()) } @@ -74,7 +78,11 @@ impl RunLifecycle for FidelityLifecycle { node: &WorkflowNode, state: &WfRunState, ) -> CoreResult { - let incoming = self.incoming_edge_data.lock().unwrap().take(); + let incoming = self + .incoming_edge_data + .lock() + .expect("fidelity mutex should not be poisoned: no code panics while holding this lock") + .take(); let gv_node = node.inner(); // 1. Fidelity resolution via resolve_fidelity: edge → node → graph default → @@ -84,7 +92,9 @@ impl RunLifecycle for FidelityLifecycle { // 2. Fidelity degradation on resume (full → summary:high) let fidelity = { - let mut degrade = self.degrade_fidelity_on_resume.lock().unwrap(); + let mut degrade = self.degrade_fidelity_on_resume.lock().expect( + "fidelity mutex should not be poisoned: no code panics while holding this lock", + ); if *degrade { *degrade = false; fidelity.degraded() @@ -180,7 +190,9 @@ impl RunLifecycle for FidelityLifecycle { let edge_data = IncomingEdgeData { edge: Arc::new(gv_edge.clone()), }; - *self.incoming_edge_data.lock().unwrap() = Some(edge_data); + *self.incoming_edge_data.lock().expect( + "fidelity mutex should not be poisoned: no code panics while holding this lock", + ) = Some(edge_data); } Ok(EdgeDecision::Continue) } diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index bc34009d9..fefaafa8c 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -94,8 +94,12 @@ pub(crate) struct GitLifecycle { impl RunLifecycle for GitLifecycle { async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> { // Reset last_git_sha (diff base parity) - *self.last_git_sha.lock().unwrap() = None; - *self.checkpoint_git_result.lock().unwrap() = None; + *self.last_git_sha.lock().expect( + "git lifecycle mutex should not be poisoned: no code panics while holding this lock", + ) = None; + *self.checkpoint_git_result.lock().expect( + "git lifecycle mutex should not be poisoned: no code panics while holding this lock", + ) = None; if let Some(meta_branch) = self.metadata_branch().map(str::to_string) { if self.metadata_writer.is_none() || self.metadata_runtime.metadata_degraded() { return Ok(()); @@ -173,7 +177,8 @@ impl RunLifecycle for GitLifecycle { // Skip git checkpoint for the start node (always empty) or if git disabled if self.start_node_id.as_deref() == Some(node_id) || self.run_options.git.is_none() { - *self.checkpoint_git_result.lock().unwrap() = None; + *self.checkpoint_git_result.lock() + .expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") = None; return Ok(()); } @@ -330,7 +335,9 @@ impl RunLifecycle for GitLifecycle { } // Save diff.patch - let prev = self.last_git_sha.lock().unwrap().clone().or_else(|| { + let prev = self.last_git_sha.lock() + .expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") + .clone().or_else(|| { self.run_options .git .as_ref() @@ -386,8 +393,10 @@ impl RunLifecycle for GitLifecycle { } // Update shared state - *self.last_git_sha.lock().unwrap() = Some(sha); - *self.checkpoint_git_result.lock().unwrap() = Some(git_result); + *self.last_git_sha.lock() + .expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") = Some(sha); + *self.checkpoint_git_result.lock() + .expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") = Some(git_result); } Err(e) => { let exec_output_tail = fabro_sandbox::default_redacted_output_tail(&e); diff --git a/lib/crates/fabro-workflow/src/lifecycle/mod.rs b/lib/crates/fabro-workflow/src/lifecycle/mod.rs index d6c70c200..2db1e8d75 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/mod.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/mod.rs @@ -391,7 +391,8 @@ impl RunLifecycle for WorkflowLifecycle { if matches!(decision, EdgeDecision::Continue) { if let Some(ref edge) = ctx.edge { if edge.inner().loop_restart() { - *self.restarted_from.lock().unwrap() = + *self.restarted_from.lock() + .expect("lifecycle mutex should not be poisoned: no code panics while holding this lock") = Some((ctx.from.to_string(), ctx.to.to_string())); } } @@ -419,7 +420,9 @@ impl RunLifecycle for WorkflowLifecycle { .on_checkpoint(node, result, next_node_id, state) .await?; // Clear checkpoint result for next checkpoint - *self.checkpoint_git_result.lock().unwrap() = None; + *self.checkpoint_git_result.lock().expect( + "lifecycle mutex should not be poisoned: no code panics while holding this lock", + ) = None; Ok(()) } diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index a5551ff72..a962cd0e5 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -804,27 +804,31 @@ impl RunSession { event if matches!(&event.body, EventBody::CheckpointCompleted(_)) => { if let EventBody::CheckpointCompleted(props) = &event.body { if let Some(sha) = props.git_commit_sha.as_ref() { - *sha_clone.lock().unwrap() = Some(sha.clone()); + *sha_clone.lock() + .expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(sha.clone()); } } } event if matches!(&event.body, EventBody::RunCompleted(_)) => { if let EventBody::RunCompleted(props) = &event.body { if let Some(sha) = props.final_git_commit_sha.as_ref() { - *sha_clone.lock().unwrap() = Some(sha.clone()); + *sha_clone.lock() + .expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(sha.clone()); } } } event if matches!(&event.body, EventBody::RunFailed(_)) => { if let EventBody::RunFailed(props) = &event.body { if let Some(sha) = props.final_git_commit_sha.as_ref() { - *sha_clone.lock().unwrap() = Some(sha.clone()); + *sha_clone.lock() + .expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(sha.clone()); } } } event if matches!(&event.body, EventBody::GitCommit(_)) => { if let EventBody::GitCommit(props) = &event.body { - *sha_clone.lock().unwrap() = Some(props.sha.clone()); + *sha_clone.lock() + .expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(props.sha.clone()); } } _ => {} @@ -894,7 +898,9 @@ impl RunSession { workflow_name: executed.graph.name.clone(), preserve_sandbox: self.preserve_sandbox, stop_on_terminal: self.stop_on_terminal, - last_git_sha: last_git_sha.lock().unwrap().clone(), + last_git_sha: last_git_sha.lock() + .expect("last_git_sha mutex should not be poisoned: no code panics while holding this lock") + .clone(), }; let pr_opts = PullRequestOptions { pr_config: self.pr_config, diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index 9210998d3..e8c4b3974 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -254,12 +254,19 @@ impl EngineServices { /// Read the current git state (if any). pub fn git_state(&self) -> Option> { - self.git_state.read().unwrap().clone() + self.git_state + .read() + .expect("git_state lock is never poisoned: no code panics while holding this lock") + .clone() } /// Set the git state for the current run. pub fn set_git_state(&self, state: Option>) { - *self.git_state.write().unwrap() = state; + *self + .git_state + .write() + .expect("git_state lock is never poisoned: no code panics while holding this lock") = + state; } /// Test-only default: empty registry and cross-phase services.