fix: resolve 9 code review issues with TDD-verified fixes

HIGH:
- Consolidate execute_tool_calls_with into execute_tool_calls:
  execute_tool_calls now accepts &[ToolCall] directly, new
  execute_tool_calls_from_msg wrapper preserves old extraction API.
  Removed 42-line duplicate from run.rs.

MEDIUM:
- Storm custom tool predicates via LoopConfig.storm_mutating_tools
  and LoopConfig.storm_exempt_tools. storm_for_config() merges
  custom lists with built-in defaults.
- Heal in run_agent_loop_continue: matches spawn_loop_runner path,
  heals before entering run_loop.
- Extract CHARS_PER_TOKEN_ESTIMATE constant to context_manager.rs
  for shared use by run.rs and session/mod.rs.
- Remove dead code: FoldResult struct, FOLD_SUMMARY_TIMEOUT_SECS.

LOW:
- Guard ctx_max==0 in estimate_turn_start (matches decide_after_usage).
- Guard max_chars<2 in truncate_for_model: early return for degenerate
  inputs.
- Make default_mutating/default_exempt pub for storm_for_config reuse.

Author: Yogthos

src/agent/agent_loop/context_manager.rs

@@ -54,8 +54,10 @@ pub const FORCE_SUMMARY_THRESHOLD: f64 = 0.8;
 /// paste).
 pub const TURN_START_FOLD_THRESHOLD: f64 = 0.9;
 
-/// Hard deadline for fold summary requests (seconds).
-pub const FOLD_SUMMARY_TIMEOUT_SECS: u64 = 15;
+/// Approximate characters-per-token ratio for estimation when
+/// actual token counts aren't available. Used by both run.rs's
+/// turn-start heuristic and Session::estimate_message_tokens.
+pub const CHARS_PER_TOKEN_ESTIMATE: u64 = 4;
 
 // ================================================================
 // Data types — port of context-manager.ts:67-85
@@ -88,15 +90,6 @@ pub struct PostUsageDecision {
     pub aggressive: bool,
 }
 
-/// Result of an attempted fold.
-#[derive(Debug, Clone)]
-pub struct FoldResult {
-    pub folded: bool,
-    pub before_messages: usize,
-    pub after_messages: usize,
-    pub summary_chars: usize,
-}
-
 /// Turn-start estimate result.
 #[derive(Debug, Clone, Copy)]
 pub struct TurnStartEstimate {
@@ -208,10 +201,15 @@ pub fn decide_after_usage(
 ///   (messages + tools + system prompt).
 /// `ctx_max`: the model's context window size in tokens.
 pub fn estimate_turn_start(estimate_tokens: u64, ctx_max: u64) -> TurnStartEstimate {
+    let ratio = if ctx_max == 0 {
+        f64::INFINITY
+    } else {
+        estimate_tokens as f64 / ctx_max as f64
+    };
     TurnStartEstimate {
         estimate_tokens,
         ctx_max,
-        ratio: estimate_tokens as f64 / ctx_max as f64,
+        ratio,
     }
 }
 

src/agent/agent_loop/heal.rs

@@ -70,15 +70,12 @@ pub fn shrink_oversized_tool_results(messages: &[Value], max_chars: usize) -> He
 /// Truncate a string to `max_chars` while keeping the beginning
 /// more useful than the end.
 fn truncate_for_model(content: &str, max_chars: usize) -> String {
-    // Keep first 70% from the top (most of the output),
-    // last 30% from the tail (likely tail errors or summaries).
+    if content.len() <= max_chars || max_chars < 2 {
+        return content.to_string();
+    }
     let head_pct = 0.7;
     let head_chars = (max_chars as f64 * head_pct) as usize;
     let tail_chars = max_chars.saturating_sub(head_chars);
-
-    if content.len() <= max_chars {
-        return content.to_string();
-    }
     let head = &content[..content
         .char_indices()
         .nth(head_chars)

src/agent/agent_loop/integration.rs

@@ -397,6 +397,8 @@ pub fn spawn_loop_runner(cfg: LoopSpawnConfig) -> LoopRunner {
         provider_name: cfg.provider_name.clone(),
         model_name: cfg.model_name.clone(),
         compact_model: None,
+        storm_mutating_tools: None,
+        storm_exempt_tools: None,
     };
 
     #[cfg(feature = "plugin")]

src/agent/agent_loop/mod.rs

@@ -83,8 +83,8 @@ pub use tool_input_repair::{
     RepairKind, RepairResult, format_structured_error, is_path_field_name, validate_and_repair,
 };
 pub use tools::{
-    ExecutedToolCallBatch, ToolCall, execute_tool_calls, execute_tool_calls_parallel,
-    execute_tool_calls_sequential, extract_tool_calls,
+    ExecutedToolCallBatch, ToolCall, execute_tool_calls, execute_tool_calls_from_msg,
+    execute_tool_calls_parallel, execute_tool_calls_sequential, extract_tool_calls,
 };
 pub use types::{
     Context, ConvertToLlmFn, GetApiKeyFn, LoopConfig, QueueMode, ThinkingLevel, ToolExecutionMode,

src/agent/agent_loop/run.rs

@@ -80,6 +80,30 @@ impl std::fmt::Display for LoopError {
 
 impl std::error::Error for LoopError {}
 
+/// Build a `StormBreaker` from `LoopConfig`, merging custom
+/// mutating/exempt tool name lists with the built-in defaults.
+fn storm_for_config(config: &LoopConfig) -> StormBreaker {
+    let has_custom = config.storm_mutating_tools.is_some() || config.storm_exempt_tools.is_some();
+    if !has_custom {
+        return StormBreaker::default();
+    }
+    let mutating: Option<Box<dyn Fn(&super::tools::ToolCall) -> bool + Send + Sync>> =
+        config.storm_mutating_tools.as_ref().map(|extras| {
+            let extra_set: std::collections::HashSet<String> = extras.iter().cloned().collect();
+            Box::new(move |c: &super::tools::ToolCall| {
+                super::storm::default_mutating(c) || extra_set.contains(&c.name)
+            }) as Box<dyn Fn(&super::tools::ToolCall) -> bool + Send + Sync>
+        });
+    let exempt: Option<Box<dyn Fn(&super::tools::ToolCall) -> bool + Send + Sync>> =
+        config.storm_exempt_tools.as_ref().map(|extras| {
+            let extra_set: std::collections::HashSet<String> = extras.iter().cloned().collect();
+            Box::new(move |c: &super::tools::ToolCall| {
+                super::storm::default_exempt(c) || extra_set.contains(&c.name)
+            }) as Box<dyn Fn(&super::tools::ToolCall) -> bool + Send + Sync>
+        });
+    StormBreaker::new(6, 3, mutating, exempt)
+}
+
 /// Public entry point: start a new run from one or more prompt
 /// messages. Faithful port of pi `runAgentLoop` (agent-loop.ts:95).
 ///
@@ -133,7 +157,7 @@ pub async fn run_agent_loop(
 ///   - otherwise → emit agent_start + turn_start, enter loop with
 ///     newMessages = [] (does NOT re-emit user-message events)
 pub async fn run_agent_loop_continue(
-    context: Context,
+    mut context: Context,
     config: LoopConfig,
     signal: AbortSignal,
     emit: &mpsc::Sender<LoopEvent>,
@@ -143,10 +167,7 @@ pub async fn run_agent_loop_continue(
     if context.messages.is_empty() {
         return Err(LoopError::NoMessages);
     }
-    // Pi lines 131-133: last-message role check. Phase 4 reads
-    // the role string from the placeholder `Vec<Value>` shape
-    // since that's what context.messages carries. Phase ??? may
-    // substitute typed messages.
+    // Pi lines 131-133: last-message role check.
     let last_role = context
         .messages
         .last()
@@ -157,6 +178,15 @@ pub async fn run_agent_loop_continue(
         return Err(LoopError::CannotContinueFromAssistant);
     }
 
+    // Heal messages before entering the loop (shrink oversized tool
+    // results, drop unpaired tool calls). Matches the healing done
+    // in spawn_loop_runner for the session-restore path.
+    let heal_result =
+        super::heal::heal_loaded_messages(&context.messages, super::heal::DEFAULT_MAX_RESULT_CHARS);
+    if heal_result.healed_count > 0 {
+        context.messages = heal_result.messages;
+    }
+
     // Pi lines 135-139: newMessages = []; emit agent_start +
     // turn_start; enter loop.
     let new_messages: Vec<LoopMessage> = Vec::new();
@@ -192,7 +222,7 @@ pub async fn run_loop(
     // Storm breaker: tracks (tool_name, args) repeats to detect
     // stuck-in-a-loop behavior. Reset each new user turn.
     // Port of Reasonix `repair/index.ts:38-46` + `loop.ts:621`.
-    let mut storm = StormBreaker::default();
+    let mut storm = storm_for_config(&config);
 
     // Inflight set: authoritative running-id tracker.
     // UI cards consult `inflight.has(call_id)` to derive spinner state.
@@ -415,9 +445,10 @@ pub async fn run_loop(
                     has_more_tool_calls = false;
                 }
 
-                // Dispatch surviving calls.
+                // Dispatch surviving calls through the unified dispatch.
+                // `execute_tool_calls` takes pre-extracted tool calls.
                 if !surviving_calls.is_empty() {
-                    let batch = execute_tool_calls_with(
+                    let batch = super::tools::execute_tool_calls(
                         &current_context,
                         &assistant_msg,
                         &surviving_calls,
@@ -588,53 +619,6 @@ fn extract_tool_calls_from(msg: &AssistantMessage) -> Vec<super::tools::ToolCall
     super::tools::extract_tool_calls(msg)
 }
 
-/// Dispatch pre-filtered tool calls (after storm breaker has
-/// removed suppressed calls). Mirrors the dispatch logic in
-/// `execute_tool_calls` but takes the calls directly instead of
-/// extracting from the assistant message.
-async fn execute_tool_calls_with(
-    context: &Context,
-    assistant_message: &AssistantMessage,
-    tool_calls: &[super::tools::ToolCall],
-    config: &LoopConfig,
-    signal: &AbortSignal,
-    emit: &mpsc::Sender<LoopEvent>,
-    inflight: &InflightSet,
-) -> super::tools::ExecutedToolCallBatch {
-    use super::ExecutedToolCallBatch;
-    let has_sequential = tool_calls.iter().any(|tc| {
-        context
-            .tools
-            .iter()
-            .find(|t| t.name() == tc.name)
-            .and_then(|t| t.execution_mode())
-            == Some(super::ToolExecutionMode::Sequential)
-    });
-    if config.tool_execution == super::ToolExecutionMode::Sequential || has_sequential {
-        super::tools::execute_tool_calls_sequential(
-            context,
-            assistant_message,
-            tool_calls,
-            config,
-            signal,
-            emit,
-            inflight,
-        )
-        .await
-    } else {
-        super::tools::execute_tool_calls_parallel(
-            context,
-            assistant_message,
-            tool_calls,
-            config,
-            signal,
-            emit,
-            inflight,
-        )
-        .await
-    }
-}
-
 /// Convert a `LoopMessage` to the placeholder `Value` shape used
 /// in `Context.messages`. Mirrors `serialize_assistant` from
 /// stream.rs but covers every variant.
@@ -755,6 +739,8 @@ mod tests {
             provider_name: None,
             model_name: None,
             compact_model: None,
+            storm_mutating_tools: None,
+            storm_exempt_tools: None,
         }
     }
 

src/agent/agent_loop/steering.rs

@@ -439,6 +439,8 @@ mod tests {
             provider_name: None,
             model_name: None,
             compact_model: None,
+            storm_mutating_tools: None,
+            storm_exempt_tools: None,
         };
         config.get_steering_messages = Some(steering_from_queue(queue.clone(), QueueMode::All));
 

src/agent/agent_loop/storm.rs

@@ -196,7 +196,7 @@ impl StormBreaker {
 
 /// Built-in mutating tools: calls that change filesystem state.
 /// Kept in sync with `crate::agent::tools::BUILTIN_TOOL_NAMES`.
-fn default_mutating(call: &ToolCall) -> bool {
+pub fn default_mutating(call: &ToolCall) -> bool {
     matches!(
         call.name.as_str(),
         "write" | "edit" | "bash" | "apply_patch"
@@ -209,7 +209,7 @@ fn default_mutating(call: &ToolCall) -> bool {
 /// `find_callers` / `find_callees` are behind `#[cfg(feature = "semantic")]`
 /// but listing them here is harmless — the match simply won't fire
 /// when the feature is off.
-fn default_exempt(call: &ToolCall) -> bool {
+pub fn default_exempt(call: &ToolCall) -> bool {
     matches!(
         call.name.as_str(),
         "read"

src/agent/agent_loop/stream.rs

@@ -375,6 +375,8 @@ mod tests {
             provider_name: None,
             model_name: None,
             compact_model: None,
+            storm_mutating_tools: None,
+            storm_exempt_tools: None,
         }
     }
 
@@ -669,6 +671,8 @@ mod tests {
             provider_name: None,
             model_name: None,
             compact_model: None,
+            storm_mutating_tools: None,
+            storm_exempt_tools: None,
         };
         let signal = AbortSignal::new();
         let (tx, mut rx) = mpsc::channel::<LoopEvent>(32);

src/agent/agent_loop/tools.rs

@@ -705,12 +705,12 @@ pub async fn execute_tool_calls_parallel(
 pub async fn execute_tool_calls(
     context: &Context,
     assistant_message: &AssistantMessage,
+    tool_calls: &[ToolCall],
     config: &LoopConfig,
     signal: &AbortSignal,
     emit: &mpsc::Sender<LoopEvent>,
     inflight: &InflightSet,
 ) -> ExecutedToolCallBatch {
-    let tool_calls = extract_tool_calls(assistant_message);
     let has_sequential = tool_calls.iter().any(|tc| {
         context
             .tools
@@ -744,6 +744,29 @@ pub async fn execute_tool_calls(
     }
 }
 
+/// Convenience: extract tool calls from the assistant message, then
+/// dispatch through [`execute_tool_calls`].
+pub async fn execute_tool_calls_from_msg(
+    context: &Context,
+    assistant_message: &AssistantMessage,
+    config: &LoopConfig,
+    signal: &AbortSignal,
+    emit: &mpsc::Sender<LoopEvent>,
+    inflight: &InflightSet,
+) -> ExecutedToolCallBatch {
+    let tool_calls = extract_tool_calls(assistant_message);
+    execute_tool_calls(
+        context,
+        assistant_message,
+        &tool_calls,
+        config,
+        signal,
+        emit,
+        inflight,
+    )
+    .await
+}
+
 /// Extract `ToolCall`s from an assistant message's content. Port
 /// of pi line 380 `message.content.filter((c) => c.type ===
 /// "toolCall")` adapted to our typed enum.
@@ -989,6 +1012,8 @@ mod tests {
             provider_name: None,
             model_name: None,
             compact_model: None,
+            storm_mutating_tools: None,
+            storm_exempt_tools: None,
         }
     }
 
@@ -1530,7 +1555,7 @@ mod tests {
 
         let (tx, _rx) = mpsc::channel::<LoopEvent>(128);
         let signal = AbortSignal::new();
-        let batch = execute_tool_calls(
+        let batch = execute_tool_calls_from_msg(
             &context,
             &assistant,
             &config,
@@ -1594,7 +1619,7 @@ mod tests {
 
         let (tx, _rx) = mpsc::channel::<LoopEvent>(128);
         let signal = AbortSignal::new();
-        let _ = execute_tool_calls(
+        let _ = execute_tool_calls_from_msg(
             &context,
             &assistant,
             &config,
@@ -1630,7 +1655,7 @@ mod tests {
 
         let (tx, _rx) = mpsc::channel::<LoopEvent>(128);
         let signal = AbortSignal::new();
-        let _ = execute_tool_calls(
+        let _ = execute_tool_calls_from_msg(
             &context,
             &assistant,
             &config,
@@ -1685,7 +1710,7 @@ mod tests {
 
         let (tx, _rx) = mpsc::channel::<LoopEvent>(128);
         let signal = AbortSignal::new();
-        let batch = execute_tool_calls(
+        let batch = execute_tool_calls_from_msg(
             &context,
             &assistant,
             &config,

src/agent/agent_loop/types.rs

@@ -287,6 +287,16 @@ pub struct LoopConfig {
     /// `LoopSpawnConfig` / `provider.rs` plumbing. Until then this
     /// field is accepted but not acted on.
     pub compact_model: Option<String>,
+
+    /// Additional tool names to treat as mutating (clears read-only
+    /// entries from the storm breaker window). Built-in defaults
+    /// (`write`, `edit`, `bash`, `apply_patch`) are always included.
+    pub storm_mutating_tools: Option<Vec<String>>,
+
+    /// Additional tool names to treat as storm-exempt (never
+    /// suppressed regardless of repetition). Built-in defaults
+    /// (`read`, `list_dir`, `grep`, etc.) are always included.
+    pub storm_exempt_tools: Option<Vec<String>>,
 }
 
 /// `convertToLlm` signature. Synchronous in pi (returns
@@ -389,6 +399,8 @@ impl Clone for LoopConfig {
             provider_name: self.provider_name.clone(),
             model_name: self.model_name.clone(),
             compact_model: self.compact_model.clone(),
+            storm_mutating_tools: self.storm_mutating_tools.clone(),
+            storm_exempt_tools: self.storm_exempt_tools.clone(),
         }
     }
 }