From 54372575215b4ef55c827245ed3e6f45b207687b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 09:17:47 -0400 Subject: [PATCH] refactor: unify metadata snapshot failure helpers and drop floor_char_boundary copies Groups the MetadataSnapshotFailed event payload into a MetadataSnapshotFailure struct and replaces the two near-identical 11-arg emit_metadata_snapshot_failed helpers in lifecycle/git.rs and pipeline/finalize.rs with one shared helper in sandbox_metadata.rs. Both #[allow(too_many_arguments)] blocks are removed. Also deletes two hand-written floor_char_boundary copies (fabro-agent and fabro-sandbox) in favor of the stable str::floor_char_boundary, matching how most existing call sites already use it. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-agent/src/cli.rs | 6 +- lib/crates/fabro-agent/src/compaction.rs | 8 +- lib/crates/fabro-agent/src/lib.rs | 4 +- lib/crates/fabro-agent/src/truncation.rs | 37 +----- lib/crates/fabro-sandbox/src/sandbox.rs | 13 +- .../fabro-workflow/src/handler/agent.rs | 2 +- .../fabro-workflow/src/lifecycle/git.rs | 116 ++++++++---------- .../fabro-workflow/src/pipeline/finalize.rs | 88 +++++-------- .../fabro-workflow/src/sandbox_metadata.rs | 44 +++++++ 9 files changed, 137 insertions(+), 181 deletions(-) diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index 363e88051..aeec4e2b9 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -30,7 +30,7 @@ use crate::subagent::{SessionFactory, SubAgentManager}; use crate::tools::WebFetchSummarizer; use crate::{ AgentEvent, AgentProfile, AnthropicProfile, GeminiProfile, LocalSandbox, OpenAiProfile, - Sandbox, Session, SessionOptions, Turn, truncation, + Sandbox, Session, SessionOptions, Turn, }; /// Public arguments for the agent command, usable from an external CLI. @@ -288,7 +288,7 @@ fn format_tool_args(args: &serde_json::Value, cwd: &str) -> String { serde_json::Value::String(s) => { let s = s.strip_prefix(&cwd_prefix).unwrap_or(s); let display = if s.len() > 80 { - format!("{}...", &s[..truncation::floor_char_boundary(s, 77)]) + format!("{}...", &s[..s.floor_char_boundary(77)]) } else { s.to_string() }; @@ -671,7 +671,7 @@ pub async fn run_with_args_and_client( .. } => { let task_preview = if task.len() > 60 { - &task[..truncation::floor_char_boundary(task, 60)] + &task[..task.floor_char_boundary(60)] } else { task }; diff --git a/lib/crates/fabro-agent/src/compaction.rs b/lib/crates/fabro-agent/src/compaction.rs index ab76391d9..d7c49c552 100644 --- a/lib/crates/fabro-agent/src/compaction.rs +++ b/lib/crates/fabro-agent/src/compaction.rs @@ -9,7 +9,6 @@ use crate::error::Error; use crate::event::Emitter; use crate::file_tracker::FileTracker; use crate::history::History; -use crate::truncation; use crate::types::{AgentEvent, Turn}; /// Check whether the context window usage exceeds the configured threshold. @@ -210,10 +209,7 @@ pub fn render_turns_for_summary(turns: &[Turn]) -> String { for tc in tool_calls { let args_str = tc.arguments.to_string(); let truncated = if args_str.len() > 500 { - format!( - "{}...", - &args_str[..truncation::floor_char_boundary(&args_str, 500)] - ) + format!("{}...", &args_str[..args_str.floor_char_boundary(500)]) } else { args_str }; @@ -226,7 +222,7 @@ pub fn render_turns_for_summary(turns: &[Turn]) -> String { let truncated = if content_str.len() > 500 { format!( "{}...", - &content_str[..truncation::floor_char_boundary(&content_str, 500)] + &content_str[..content_str.floor_char_boundary(500)] ) } else { content_str diff --git a/lib/crates/fabro-agent/src/lib.rs b/lib/crates/fabro-agent/src/lib.rs index b0fe24dab..4b8ffba68 100644 --- a/lib/crates/fabro-agent/src/lib.rs +++ b/lib/crates/fabro-agent/src/lib.rs @@ -54,9 +54,7 @@ pub use tools::{ WebFetchSummarizer, make_edit_file_tool, make_glob_tool, make_grep_tool, make_read_file_tool, make_shell_tool, make_shell_tool_with_config, make_write_file_tool, register_core_tools, }; -pub use truncation::{ - TruncationMode, floor_char_boundary, truncate_lines, truncate_output, truncate_tool_output, -}; +pub use truncation::{TruncationMode, truncate_lines, truncate_output, truncate_tool_output}; pub use types::{AgentEvent, SessionEvent, SessionState, Turn}; #[cfg(test)] diff --git a/lib/crates/fabro-agent/src/truncation.rs b/lib/crates/fabro-agent/src/truncation.rs index 78cfa1152..d5cb0201e 100644 --- a/lib/crates/fabro-agent/src/truncation.rs +++ b/lib/crates/fabro-agent/src/truncation.rs @@ -1,19 +1,5 @@ use crate::config::SessionOptions; -/// Round a byte index down to the nearest UTF-8 char boundary. -/// Stable equivalent of `str::floor_char_boundary` (nightly-only). -#[must_use] -pub fn floor_char_boundary(s: &str, index: usize) -> usize { - if index >= s.len() { - return s.len(); - } - let mut i = index; - while i > 0 && !s.is_char_boundary(i) { - i -= 1; - } - i -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum TruncationMode { HeadTail, @@ -58,8 +44,8 @@ pub fn truncate_output(output: &str, max_chars: usize, mode: TruncationMode) -> match mode { TruncationMode::HeadTail => { let half = max_chars / 2; - let head_end = floor_char_boundary(output, half); - let tail_start = floor_char_boundary(output, output.len() - half); + let head_end = output.floor_char_boundary(half); + let tail_start = output.floor_char_boundary(output.len() - half); let head = &output[..head_end]; let tail = &output[tail_start..]; format!( @@ -69,7 +55,7 @@ pub fn truncate_output(output: &str, max_chars: usize, mode: TruncationMode) -> ) } TruncationMode::Tail => { - let tail_start = floor_char_boundary(output, output.len() - max_chars); + let tail_start = output.floor_char_boundary(output.len() - max_chars); let tail = &output[tail_start..]; format!( "[WARNING: Tool output was truncated. First {removed} characters were removed. \ @@ -250,23 +236,6 @@ mod tests { assert_eq!(result, output); } - #[test] - fn floor_char_boundary_ascii() { - assert_eq!(floor_char_boundary("hello", 3), 3); - assert_eq!(floor_char_boundary("hello", 10), 5); - assert_eq!(floor_char_boundary("hello", 0), 0); - } - - #[test] - fn floor_char_boundary_multibyte() { - // ✅ is 3 bytes (E2 9C 85) - let s = "a✅b"; - assert_eq!(floor_char_boundary(s, 1), 1); // just past 'a' - assert_eq!(floor_char_boundary(s, 2), 1); // inside ✅, rounds down to 'a' - assert_eq!(floor_char_boundary(s, 3), 1); // still inside ✅ - assert_eq!(floor_char_boundary(s, 4), 4); // at 'b' - } - #[test] fn truncate_output_multibyte_no_panic() { let output = "✅".repeat(100); // 300 bytes diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index 6926751de..0abe33235 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -497,7 +497,7 @@ fn redacted_tail(text: &str, max_bytes: usize) -> (Option, bool) { let sanitized = sanitize_exec_output(&redacted); let truncated = sanitized.len() > max_bytes; let start = if truncated { - floor_char_boundary(&sanitized, sanitized.len() - max_bytes) + sanitized.floor_char_boundary(sanitized.len() - max_bytes) } else { 0 }; @@ -547,17 +547,6 @@ fn sanitize_exec_output(text: &str) -> String { sanitized } -fn floor_char_boundary(text: &str, index: usize) -> usize { - if index >= text.len() { - return text.len(); - } - let mut boundary = index; - while boundary > 0 && !text.is_char_boundary(boundary) { - boundary -= 1; - } - boundary -} - #[derive(Debug, Clone)] pub struct ExecStreamingResult { pub result: ExecResult, diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 7fdfc3d27..32d25ddf0 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -194,7 +194,7 @@ pub(crate) fn truncate(s: &str, max_chars: usize) -> &str { if s.len() <= max_chars { s } else { - &s[..fabro_agent::floor_char_boundary(s, max_chars)] + &s[..s.floor_char_boundary(max_chars)] } } diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index 329fdad50..80ecff921 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -21,7 +21,10 @@ use crate::run_dump::RunDump; use crate::run_options::RunOptions; use crate::runtime_store::RunStoreHandle; use crate::sandbox_git::{checked_git_checkpoint, git_diff}; -use crate::sandbox_metadata::{MetadataSnapshot, SandboxGitRuntime, SandboxMetadataWriter}; +use crate::sandbox_metadata::{ + MetadataSnapshot, MetadataSnapshotFailure, SandboxGitRuntime, SandboxMetadataWriter, + emit_metadata_snapshot_failed, +}; type WfRunState = ExecutionState>; type WfNodeResult = NodeResult>; @@ -106,17 +109,20 @@ impl RunLifecycle for GitLifecycle { } Err(err) => { let message = format!("failed to load run state for metadata init: {err}"); - self.emit_metadata_snapshot_failed( + emit_metadata_snapshot_failed( + &self.emitter, phase, &meta_branch, started, - MetadataSnapshotFailureKind::LoadState, - message.clone(), - collect_causes(err.as_ref()), - None, - None, - None, - None, + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::LoadState, + error: message.clone(), + causes: collect_causes(err.as_ref()), + commit_sha: None, + entry_count: None, + bytes: None, + exec_output_tail: None, + }, None, ); self.emit_metadata_warning("checkpoint_metadata_write_failed", message); @@ -176,17 +182,20 @@ impl RunLifecycle for GitLifecycle { Err(err) => { let message = format!("failed to load run state for metadata checkpoint: {err}"); - self.emit_metadata_snapshot_failed( + emit_metadata_snapshot_failed( + &self.emitter, phase, &meta_branch, started, - MetadataSnapshotFailureKind::LoadState, - message.clone(), - collect_causes(err.as_ref()), - None, - None, - None, - None, + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::LoadState, + error: message.clone(), + causes: collect_causes(err.as_ref()), + commit_sha: None, + entry_count: None, + bytes: None, + exec_output_tail: None, + }, Some(&scope), ); self.emit_metadata_warning("checkpoint_metadata_write_failed", message); @@ -328,17 +337,20 @@ impl GitLifecycle { let message = format!( "failed to push metadata ref refs/heads/{meta_branch}: {push_error}" ); - self.emit_metadata_snapshot_failed( + emit_metadata_snapshot_failed( + &self.emitter, phase, meta_branch, started, - MetadataSnapshotFailureKind::Push, - message.clone(), - Vec::new(), - Some(snapshot.commit_sha.clone()), - Some(snapshot.entry_count), - Some(snapshot.bytes), - push_error.exec_output_tail(), + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::Push, + error: message.clone(), + causes: Vec::new(), + commit_sha: Some(snapshot.commit_sha.clone()), + entry_count: Some(snapshot.entry_count), + bytes: Some(snapshot.bytes), + exec_output_tail: push_error.exec_output_tail(), + }, scope, ); self.emit_metadata_warning("checkpoint_metadata_push_failed", message); @@ -355,17 +367,20 @@ impl GitLifecycle { } Err(err) => { let message = format!("failed to write checkpoint metadata: {err}"); - self.emit_metadata_snapshot_failed( + emit_metadata_snapshot_failed( + &self.emitter, phase, meta_branch, started, - MetadataSnapshotFailureKind::Write, - message.clone(), - collect_causes(&err), - None, - None, - None, - err.exec_output_tail(), + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::Write, + error: message.clone(), + causes: collect_causes(&err), + commit_sha: None, + entry_count: None, + bytes: None, + exec_output_tail: err.exec_output_tail(), + }, scope, ); self.emit_metadata_warning("checkpoint_metadata_write_failed", message); @@ -410,41 +425,6 @@ impl GitLifecycle { ); } - #[allow( - clippy::too_many_arguments, - reason = "Metadata failure event carries the full event contract explicitly." - )] - fn emit_metadata_snapshot_failed( - &self, - phase: MetadataSnapshotPhase, - branch: &str, - started: Instant, - failure_kind: MetadataSnapshotFailureKind, - error: String, - causes: Vec, - commit_sha: Option, - entry_count: Option, - bytes: Option, - exec_output_tail: Option, - scope: Option<&StageScope>, - ) { - self.emit_metadata_snapshot_event( - &Event::MetadataSnapshotFailed { - phase, - branch: branch.to_string(), - duration_ms: elapsed_ms(started), - failure_kind, - error, - causes, - commit_sha, - entry_count, - bytes, - exec_output_tail, - }, - scope, - ); - } - fn emit_metadata_snapshot_event(&self, event: &Event, scope: Option<&StageScope>) { if let Some(scope) = scope { self.emitter.emit_scoped(event, scope); diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index b11ac0603..d0977d887 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -16,7 +16,9 @@ use crate::run_options::RunOptions; use crate::run_status::{FailureReason, RunStatus, SuccessReason}; use crate::runtime_store::RunStoreHandle; use crate::sandbox_git::git_diff_with_timeout; -use crate::sandbox_metadata::{MetadataSnapshot, SandboxMetadataWriter}; +use crate::sandbox_metadata::{ + MetadataSnapshot, MetadataSnapshotFailure, SandboxMetadataWriter, emit_metadata_snapshot_failed, +}; use crate::services::RunServices; pub fn classify_engine_result( @@ -171,16 +173,19 @@ pub async fn write_finalize_commit( Err(err) => { let message = format!("failed to load run state for final metadata snapshot: {err}"); emit_metadata_snapshot_failed( - services, + &services.emitter, phase, meta_branch, started, - MetadataSnapshotFailureKind::LoadState, - message.clone(), - collect_causes(err.as_ref()), - None, - None, - None, + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::LoadState, + error: message.clone(), + causes: collect_causes(err.as_ref()), + commit_sha: None, + entry_count: None, + bytes: None, + exec_output_tail: None, + }, None, ); emit_metadata_warning(services, "checkpoint_metadata_write_failed", message); @@ -203,17 +208,20 @@ pub async fn write_finalize_commit( let message = format!("failed to push metadata ref refs/heads/{meta_branch}: {push_error}"); emit_metadata_snapshot_failed( - services, + &services.emitter, phase, meta_branch, started, - MetadataSnapshotFailureKind::Push, - message.clone(), - Vec::new(), - Some(snapshot.commit_sha.clone()), - Some(snapshot.entry_count), - Some(snapshot.bytes), - push_error.exec_output_tail(), + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::Push, + error: message.clone(), + causes: Vec::new(), + commit_sha: Some(snapshot.commit_sha.clone()), + entry_count: Some(snapshot.entry_count), + bytes: Some(snapshot.bytes), + exec_output_tail: push_error.exec_output_tail(), + }, + None, ); emit_metadata_warning(services, "checkpoint_metadata_push_failed", message); } else { @@ -223,17 +231,20 @@ pub async fn write_finalize_commit( Err(err) => { let message = format!("failed to write final checkpoint metadata: {err}"); emit_metadata_snapshot_failed( - services, + &services.emitter, phase, meta_branch, started, - MetadataSnapshotFailureKind::Write, - message.clone(), - collect_causes(&err), + MetadataSnapshotFailure { + kind: MetadataSnapshotFailureKind::Write, + error: message.clone(), + causes: collect_causes(&err), + commit_sha: None, + entry_count: None, + bytes: None, + exec_output_tail: err.exec_output_tail(), + }, None, - None, - None, - err.exec_output_tail(), ); emit_metadata_warning(services, "checkpoint_metadata_write_failed", message); } @@ -268,37 +279,6 @@ fn emit_metadata_snapshot_completed( }); } -#[allow( - clippy::too_many_arguments, - reason = "Metadata failure event carries the full event contract explicitly." -)] -fn emit_metadata_snapshot_failed( - services: &RunServices, - phase: MetadataSnapshotPhase, - branch: &str, - started: Instant, - failure_kind: MetadataSnapshotFailureKind, - error: String, - causes: Vec, - commit_sha: Option, - entry_count: Option, - bytes: Option, - exec_output_tail: Option, -) { - services.emitter.emit(&Event::MetadataSnapshotFailed { - phase, - branch: branch.to_string(), - duration_ms: elapsed_ms(started), - failure_kind, - error, - causes, - commit_sha, - entry_count, - bytes, - exec_output_tail, - }); -} - fn emit_metadata_warning(services: &RunServices, code: &str, message: String) { if services.metadata_runtime.mark_metadata_degraded() { services.emitter.notice(RunNoticeLevel::Warn, code, message); diff --git a/lib/crates/fabro-workflow/src/sandbox_metadata.rs b/lib/crates/fabro-workflow/src/sandbox_metadata.rs index 619a2802a..004d01841 100644 --- a/lib/crates/fabro-workflow/src/sandbox_metadata.rs +++ b/lib/crates/fabro-workflow/src/sandbox_metadata.rs @@ -1,12 +1,16 @@ use std::collections::HashMap; use std::fmt::Write as _; use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Instant; use fabro_agent::Sandbox; use fabro_sandbox::shell_quote; +use fabro_types::run_event::{MetadataSnapshotFailureKind, MetadataSnapshotPhase}; +use fabro_util::time::elapsed_ms; use tokio::fs; use tokio::sync::OnceCell; +use crate::event::{Emitter, Event, StageScope}; use crate::git::{GitAuthor, META_BRANCH_PREFIX}; use crate::run_dump::RunDump; use crate::sandbox_git::GIT_REMOTE; @@ -94,6 +98,46 @@ pub(crate) struct MetadataSnapshot { pub bytes: u64, } +/// Full payload for a `MetadataSnapshotFailed` event, grouped so the emit +/// helper stays under clippy's argument threshold. +#[derive(Debug)] +pub(crate) struct MetadataSnapshotFailure { + pub kind: MetadataSnapshotFailureKind, + pub error: String, + pub causes: Vec, + pub commit_sha: Option, + pub entry_count: Option, + pub bytes: Option, + pub exec_output_tail: Option, +} + +pub(crate) fn emit_metadata_snapshot_failed( + emitter: &Emitter, + phase: MetadataSnapshotPhase, + branch: &str, + started: Instant, + failure: MetadataSnapshotFailure, + scope: Option<&StageScope>, +) { + let event = Event::MetadataSnapshotFailed { + phase, + branch: branch.to_string(), + duration_ms: elapsed_ms(started), + failure_kind: failure.kind, + error: failure.error, + causes: failure.causes, + commit_sha: failure.commit_sha, + entry_count: failure.entry_count, + bytes: failure.bytes, + exec_output_tail: failure.exec_output_tail, + }; + if let Some(scope) = scope { + emitter.emit_scoped(&event, scope); + } else { + emitter.emit(&event); + } +} + impl<'a> SandboxMetadataWriter<'a> { pub(crate) fn new( sandbox: &'a dyn Sandbox,