From bd0d47ba4d9af520530bb5c5f44a0c8674370f44 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 19 Apr 2026 20:48:44 -0400 Subject: [PATCH] refactor(unwrap): clean runtime hotspot call sites --- lib/crates/fabro-cli/src/commands/run/logs.rs | 36 +++++++------ lib/crates/fabro-core/src/context.rs | 2 +- lib/crates/fabro-server/src/run_manifest.rs | 13 +++-- lib/crates/fabro-util/src/backoff.rs | 5 +- lib/crates/fabro-util/src/check_report.rs | 53 +++++++++++-------- lib/crates/fabro-util/src/redact/entropy.rs | 2 +- lib/crates/fabro-util/src/redact/gitleaks.rs | 2 +- lib/crates/fabro-util/src/redact/mod.rs | 4 +- lib/crates/fabro-workflow/src/error.rs | 13 +++-- .../fabro-workflow/src/lifecycle/event.rs | 11 ++-- .../fabro-workflow/src/pipeline/execute.rs | 8 +-- 11 files changed, 85 insertions(+), 64 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/logs.rs b/lib/crates/fabro-cli/src/commands/run/logs.rs index e7614834f..6ce41b8fb 100644 --- a/lib/crates/fabro-cli/src/commands/run/logs.rs +++ b/lib/crates/fabro-cli/src/commands/run/logs.rs @@ -141,12 +141,12 @@ fn try_parse_relative_duration(s: &str) -> Option { return None; } let (num_str, unit) = s.split_at(s.len() - 1); - let num: u64 = num_str.parse().ok()?; + let num = i64::try_from(num_str.parse::().ok()?).ok()?; match unit { - "s" => Some(chrono::Duration::seconds(i64::try_from(num).unwrap())), - "m" => Some(chrono::Duration::minutes(i64::try_from(num).unwrap())), - "h" => Some(chrono::Duration::hours(i64::try_from(num).unwrap())), - "d" => Some(chrono::Duration::days(i64::try_from(num).unwrap())), + "s" => Some(chrono::Duration::seconds(num)), + "m" => Some(chrono::Duration::minutes(num)), + "h" => Some(chrono::Duration::hours(num)), + "d" => Some(chrono::Duration::days(num)), _ => None, } } @@ -358,40 +358,39 @@ pub(crate) fn format_event_pretty(line: &str, styles: &Styles) -> Option { let total = billing .get("total_tokens") - .and_then(serde_json::Value::as_i64) + .and_then(serde_json::Value::as_u64) .unwrap_or(0); let pad = " ".repeat(ts.len() + 1); if total > 0 { lines.push(format!( "{}{}", pad, - styles.dim.apply_to(format!( - "Tokens: {}", - format_tokens(u64::try_from(total).unwrap()) - )) + styles + .dim + .apply_to(format!("Tokens: {}", format_tokens(total))) )); } if let Some(cache_read) = billing .get("cache_read_tokens") - .and_then(serde_json::Value::as_i64) + .and_then(serde_json::Value::as_u64) { let cache_write = billing .get("cache_write_tokens") - .and_then(serde_json::Value::as_i64) + .and_then(serde_json::Value::as_u64) .unwrap_or(0); lines.push(format!( "{}{}", pad, styles.dim.apply_to(format!( "Cache: {} read, {} write", - format_tokens(u64::try_from(cache_read).unwrap()), - format_tokens(u64::try_from(cache_write).unwrap()) + format_tokens(cache_read), + format_tokens(cache_write) )) )); } if let Some(reasoning) = billing .get("reasoning_tokens") - .and_then(serde_json::Value::as_i64) + .and_then(serde_json::Value::as_u64) { if reasoning > 0 { lines.push(format!( @@ -399,7 +398,7 @@ pub(crate) fn format_event_pretty(line: &str, styles: &Styles) -> Option pad, styles.dim.apply_to(format!( "Reasoning: {} tokens", - format_tokens(u64::try_from(reasoning).unwrap()) + format_tokens(reasoning) )) )); } @@ -842,6 +841,11 @@ mod tests { assert!(parse_since("notadate").is_err()); } + #[test] + fn parse_since_overflow_is_invalid() { + assert!(parse_since("9223372036854775808s").is_err()); + } + #[test] fn tail_returns_last_n_lines() { let lines: Vec = (0..10).map(|i| format!("line {i}")).collect(); diff --git a/lib/crates/fabro-core/src/context.rs b/lib/crates/fabro-core/src/context.rs index 0dc17b635..01761c2d9 100644 --- a/lib/crates/fabro-core/src/context.rs +++ b/lib/crates/fabro-core/src/context.rs @@ -66,7 +66,7 @@ impl Context { pub fn node_visit_count(&self) -> usize { self.get("internal.node_visit_count") .and_then(|v| v.as_u64()) - .map_or(0, |v| usize::try_from(v).unwrap()) + .map_or(0, |v| usize::try_from(v).unwrap_or(usize::MAX)) } } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index fe5a1a119..486600d80 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -298,7 +298,10 @@ fn resolve_manifest_dockerfile( .and_then(|sandbox| sandbox.daytona.as_mut()) .and_then(|daytona| daytona.snapshot.as_mut()) .and_then(|snapshot| snapshot.dockerfile.as_mut()); - let Some(DaytonaDockerfileLayer::Path { path }) = source else { + let Some(source) = source else { + return Ok(()); + }; + let DaytonaDockerfileLayer::Path { path } = &*source else { return Ok(()); }; let path_owned = path.clone(); @@ -311,7 +314,7 @@ fn resolve_manifest_dockerfile( .get(&logical_path) .cloned() .ok_or_else(|| anyhow!("missing bundled dockerfile: {}", logical_path.display()))?; - *source.unwrap() = DaytonaDockerfileLayer::Inline(content); + *source = DaytonaDockerfileLayer::Inline(content); Ok(()) } @@ -842,11 +845,13 @@ fn preflight_response( checks: report_to_api(report), workflow: types::PreflightWorkflowSummary { diagnostics: diagnostics_to_api(validated.diagnostics()), - edges: i64::try_from(validated.graph().edges.len()).unwrap(), + edges: i64::try_from(validated.graph().edges.len()) + .expect("graph edge count should fit in i64"), goal: validated.graph().goal().to_string(), graph_path: Some(target_path.display().to_string()), name: validated.graph().name.clone(), - nodes: i64::try_from(validated.graph().nodes.len()).unwrap(), + nodes: i64::try_from(validated.graph().nodes.len()) + .expect("graph node count should fit in i64"), }, } } diff --git a/lib/crates/fabro-util/src/backoff.rs b/lib/crates/fabro-util/src/backoff.rs index d1941899e..caecf37e1 100644 --- a/lib/crates/fabro-util/src/backoff.rs +++ b/lib/crates/fabro-util/src/backoff.rs @@ -23,9 +23,8 @@ impl Default for BackoffPolicy { impl BackoffPolicy { pub fn delay_for_attempt(&self, attempt: u32) -> Duration { - let multiplier = self - .factor - .powi(i32::try_from(attempt.saturating_sub(1)).unwrap()); + let exponent = i32::try_from(attempt.saturating_sub(1)).unwrap_or(i32::MAX); + let multiplier = self.factor.powi(exponent); let base_delay = self.initial_delay.mul_f64(multiplier); let capped = if base_delay > self.max_delay { self.max_delay diff --git a/lib/crates/fabro-util/src/check_report.rs b/lib/crates/fabro-util/src/check_report.rs index c20d307a3..051ed5b97 100644 --- a/lib/crates/fabro-util/src/check_report.rs +++ b/lib/crates/fabro-util/src/check_report.rs @@ -13,7 +13,8 @@ use crate::terminal::Styles; fn write_styled_remediation(out: &mut String, text: &str, s: &Styles) { for (i, segment) in text.split('`').enumerate() { if i % 2 == 1 { - write!(out, "{}", s.bold_cyan.apply_to(segment)).unwrap(); + write!(out, "{}", s.bold_cyan.apply_to(segment)) + .expect("writing to String should not fail"); } else { out.push_str(segment); } @@ -95,15 +96,17 @@ impl CheckReport { let show_section_headers = self.sections.len() > 1; - writeln!(out, "{}", s.bold.apply_to(&self.title)).unwrap(); - writeln!(out).unwrap(); + writeln!(out, "{}", s.bold.apply_to(&self.title)) + .expect("writing to String should not fail"); + writeln!(out).expect("writing to String should not fail"); for (i, section) in self.sections.iter().enumerate() { if show_section_headers { if i > 0 { - writeln!(out).unwrap(); + writeln!(out).expect("writing to String should not fail"); } - writeln!(out, " {}", s.dim.apply_to(§ion.title)).unwrap(); + writeln!(out, " {}", s.dim.apply_to(§ion.title)) + .expect("writing to String should not fail"); } for check in §ion.checks { @@ -120,7 +123,7 @@ impl CheckReport { s.bold.apply_to(&check.name), check.summary, ) - .unwrap(); + .expect("writing to String should not fail"); if verbose { for detail in &check.details { @@ -133,9 +136,11 @@ impl CheckReport { detail.text.clone() }; if detail.warn { - writeln!(out, " • {}", s.red.apply_to(&text)).unwrap(); + writeln!(out, " • {}", s.red.apply_to(&text)) + .expect("writing to String should not fail"); } else { - writeln!(out, " • {text}").unwrap(); + writeln!(out, " • {text}") + .expect("writing to String should not fail"); } } } @@ -143,10 +148,10 @@ impl CheckReport { } let issues = self.issue_count(); - writeln!(out).unwrap(); + writeln!(out).expect("writing to String should not fail"); if issues == 0 { - writeln!(out, "All checks passed.").unwrap(); + writeln!(out, "All checks passed.").expect("writing to String should not fail"); } else { writeln!( out, @@ -157,22 +162,23 @@ impl CheckReport { "categories" } ) - .unwrap(); + .expect("writing to String should not fail"); let errors: Vec<_> = self .all_checks() .filter(|c| c.status == CheckStatus::Error) .collect(); if !errors.is_empty() { - writeln!(out).unwrap(); - writeln!(out, "{}", s.bold.apply_to("Errors:")).unwrap(); + writeln!(out).expect("writing to String should not fail"); + writeln!(out, "{}", s.bold.apply_to("Errors:")) + .expect("writing to String should not fail"); for check in &errors { - write!(out, " • {}", check.name).unwrap(); + write!(out, " • {}", check.name).expect("writing to String should not fail"); if let Some(ref rem) = check.remediation { - write!(out, " — ").unwrap(); + write!(out, " — ").expect("writing to String should not fail"); write_styled_remediation(&mut out, rem, s); } - writeln!(out).unwrap(); + writeln!(out).expect("writing to String should not fail"); } } @@ -181,22 +187,23 @@ impl CheckReport { .filter(|c| c.status == CheckStatus::Warning) .collect(); if !warnings.is_empty() { - writeln!(out).unwrap(); - writeln!(out, "{}", s.bold.apply_to("Warnings:")).unwrap(); + writeln!(out).expect("writing to String should not fail"); + writeln!(out, "{}", s.bold.apply_to("Warnings:")) + .expect("writing to String should not fail"); for check in &warnings { - write!(out, " • {}", check.name).unwrap(); + write!(out, " • {}", check.name).expect("writing to String should not fail"); if let Some(ref rem) = check.remediation { - write!(out, " — ").unwrap(); + write!(out, " — ").expect("writing to String should not fail"); write_styled_remediation(&mut out, rem, s); } - writeln!(out).unwrap(); + writeln!(out).expect("writing to String should not fail"); } } } if let Some(footer_text) = footer { - writeln!(out).unwrap(); - writeln!(out, "{footer_text}").unwrap(); + writeln!(out).expect("writing to String should not fail"); + writeln!(out, "{footer_text}").expect("writing to String should not fail"); } out diff --git a/lib/crates/fabro-util/src/redact/entropy.rs b/lib/crates/fabro-util/src/redact/entropy.rs index bc4007796..dc744cf39 100644 --- a/lib/crates/fabro-util/src/redact/entropy.rs +++ b/lib/crates/fabro-util/src/redact/entropy.rs @@ -7,7 +7,7 @@ use super::Region; /// Matches high-entropy alphanumeric strings (10+ chars). /// Excludes `/` to avoid matching file paths as single tokens. static SECRET_PATTERN: LazyLock = - LazyLock::new(|| Regex::new(r"[A-Za-z0-9+_=-]{10,}").unwrap()); + LazyLock::new(|| Regex::new(r"[A-Za-z0-9+_=-]{10,}").expect("hardcoded regex should compile")); const ENTROPY_THRESHOLD: f64 = 4.5; diff --git a/lib/crates/fabro-util/src/redact/gitleaks.rs b/lib/crates/fabro-util/src/redact/gitleaks.rs index c98df24bc..5d8115fd2 100644 --- a/lib/crates/fabro-util/src/redact/gitleaks.rs +++ b/lib/crates/fabro-util/src/redact/gitleaks.rs @@ -166,7 +166,7 @@ impl GitleaksEngine { let secret_group = rule.secret_group(); for caps in regex.captures_iter(s) { - let full_match = caps.get(0).unwrap(); + let full_match = caps.get(0).expect("captures should include the full match"); // Get the secret: group 1 if it exists, otherwise full match let secret_match = if secret_group > 0 { diff --git a/lib/crates/fabro-util/src/redact/mod.rs b/lib/crates/fabro-util/src/redact/mod.rs index ba472c917..937f4a58b 100644 --- a/lib/crates/fabro-util/src/redact/mod.rs +++ b/lib/crates/fabro-util/src/redact/mod.rs @@ -29,7 +29,9 @@ pub fn redact_string(s: &str) -> String { // Merge overlapping regions let mut merged = vec![regions[0].clone()]; for r in ®ions[1..] { - let last = merged.last_mut().unwrap(); + let last = merged + .last_mut() + .expect("merged regions should be non-empty during overlap coalescing"); if r.start <= last.end { last.end = last.end.max(r.end); } else { diff --git a/lib/crates/fabro-workflow/src/error.rs b/lib/crates/fabro-workflow/src/error.rs index 63b37c94e..2959fba1a 100644 --- a/lib/crates/fabro-workflow/src/error.rs +++ b/lib/crates/fabro-workflow/src/error.rs @@ -141,10 +141,15 @@ pub fn normalize_failure_reason(reason: &str) -> String { use regex::Regex; - static HEX_RE: LazyLock = LazyLock::new(|| Regex::new(r"\b[0-9a-f]{7,64}\b").unwrap()); - static DIGITS_RE: LazyLock = LazyLock::new(|| Regex::new(r"\b\d+\b").unwrap()); - static COMMA_SPACE_RE: LazyLock = LazyLock::new(|| Regex::new(r",\s+").unwrap()); - static WHITESPACE_RE: LazyLock = LazyLock::new(|| Regex::new(r"\s+").unwrap()); + static HEX_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"\b[0-9a-f]{7,64}\b").expect("hardcoded regex should compile") + }); + static DIGITS_RE: LazyLock = + LazyLock::new(|| Regex::new(r"\b\d+\b").expect("hardcoded regex should compile")); + static COMMA_SPACE_RE: LazyLock = + LazyLock::new(|| Regex::new(r",\s+").expect("hardcoded regex should compile")); + static WHITESPACE_RE: LazyLock = + LazyLock::new(|| Regex::new(r"\s+").expect("hardcoded regex should compile")); let s = reason.trim().to_lowercase(); if s.is_empty() { diff --git a/lib/crates/fabro-workflow/src/lifecycle/event.rs b/lib/crates/fabro-workflow/src/lifecycle/event.rs index d4714944c..3148948ce 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/event.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/event.rs @@ -209,7 +209,7 @@ impl RunLifecycle for EventLifecycle { let stage_index = state.stage_index; let scope = stage_scope_for(state, &gv.id); - let duration_ms = u64::try_from(ctx.result.duration.as_millis()).unwrap(); + let duration_ms = crate::millis_u64(ctx.result.duration); self.emitter.emit_scoped( &Event::StageFailed { node_id: gv.id.clone(), @@ -231,9 +231,7 @@ impl RunLifecycle for EventLifecycle { index: stage_index, attempt: ctx.attempt as usize, max_attempts: ctx.result.max_attempts as usize, - delay_ms: ctx - .backoff_delay - .map_or(0, |d| u64::try_from(d.as_millis()).unwrap()), + delay_ms: ctx.backoff_delay.map_or(0, crate::millis_u64), }, &scope, ); @@ -255,7 +253,7 @@ impl RunLifecycle for EventLifecycle { let gv = node.inner(); let stage_index = state.stage_index; let scope = stage_scope_for(state, &gv.id); - let duration_ms = u64::try_from(result.duration.as_millis()).unwrap(); + let duration_ms = crate::millis_u64(result.duration); let (loop_failure_signatures, restart_failure_signatures) = snapshot_failure_signatures(&self.circuit_breaker); @@ -419,8 +417,7 @@ impl RunLifecycle for EventLifecycle { } async fn on_run_end(&self, outcome: &Outcome, state: &WfRunState) { - let duration_ms = - u64::try_from(self.run_start.lock().unwrap().elapsed().as_millis()).unwrap(); + let duration_ms = crate::millis_u64(self.run_start.lock().unwrap().elapsed()); let artifact_count = self.captured_artifact_count.load(Ordering::Relaxed); let last_sha = self.last_git_sha.lock().unwrap().clone(); let final_patch = self.final_patch.lock().unwrap().clone(); diff --git a/lib/crates/fabro-workflow/src/pipeline/execute.rs b/lib/crates/fabro-workflow/src/pipeline/execute.rs index 79f74a3c3..cf42d40d1 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute.rs @@ -231,7 +231,7 @@ pub async fn execute(init: Initialized) -> Executed { let graph_max = graph.max_node_visits(); let max_node_visits = if graph_max > 0 { - Some(usize::try_from(graph_max).unwrap()) + Some(usize::try_from(graph_max).expect("positive max_node_visits should fit in usize")) } else if run_options.dry_run_enabled() { Some(10) } else { @@ -261,9 +261,11 @@ pub async fn execute(init: Initialized) -> Executed { .unwrap_or_default() .as_millis(), ) - .unwrap(); + .unwrap_or(i64::MAX); let idle_ms = now.saturating_sub(last); - if idle_ms >= i64::try_from(stall_timeout.as_millis()).unwrap() { + let stall_timeout_ms = + i64::try_from(stall_timeout.as_millis()).unwrap_or(i64::MAX); + if idle_ms >= stall_timeout_ms { token_clone.cancel(); return; }