From cfdb87cce349dc81ebf940449d2de4dfef048b08 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Wed, 22 Apr 2026 21:16:02 -0700 Subject: [PATCH 1/2] fix: mutex poison safety, UTF-8 boundary handling, option hygiene MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace 14 mutex unwrap() calls with poison-safe into_inner() across spawn, tasks, cron, worktree, and file_read tools — panicked threads no longer cascade into process-wide lock poisoning. - Harden string slicing in query_engine truncate_json and render.rs session preview to respect UTF-8 char boundaries. - Fix unchecked rag indexer slice bound when start_byte exceeds source. - Clean up input_history up/down option handling and clipboard stdin error propagation in commands. Co-Authored-By: Arch Linux --- src/commands/mod.rs | 6 +++++- src/query_engine.rs | 6 +++++- src/rag/indexer.rs | 3 ++- src/spawn.rs | 14 +++++++------- src/tools/cron.rs | 6 +++--- src/tools/file_read.rs | 2 +- src/tools/tasks.rs | 12 ++++++------ src/tools/worktree.rs | 6 +++--- src/tui/app.rs | 21 ++++++++++++--------- src/tui/render.rs | 6 +++++- 10 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index a0421e7..f07529f 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -1851,7 +1851,11 @@ pub fn clipboard_write(text: &str) -> CommandAction { .args(args) .stdin(std::process::Stdio::piped()) .spawn()?; - child.stdin.as_mut().unwrap().write_all(text.as_bytes())?; + child + .stdin + .as_mut() + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::BrokenPipe, "stdin not available"))? + .write_all(text.as_bytes())?; child.wait() }; diff --git a/src/query_engine.rs b/src/query_engine.rs index 32fa36b..2ab40f7 100644 --- a/src/query_engine.rs +++ b/src/query_engine.rs @@ -597,7 +597,11 @@ fn truncate_json(v: &serde_json::Value, max_len: usize) -> String { if s.len() <= max_len { s } else { - format!("{}...", &s[..max_len]) + let mut end = max_len; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + format!("{}...", &s[..end]) } } diff --git a/src/rag/indexer.rs b/src/rag/indexer.rs index 31ee8d8..d2ac7d6 100644 --- a/src/rag/indexer.rs +++ b/src/rag/indexer.rs @@ -318,7 +318,8 @@ fn collect_symbols( // Extract the source text for this node let start_byte = child.start_byte(); let end_byte = child.end_byte(); - let content = &ctx.full_source[start_byte..end_byte.min(ctx.full_source.len())]; + let src_len = ctx.full_source.len(); + let content = &ctx.full_source[start_byte.min(src_len)..end_byte.min(src_len)]; // Cap chunk size at 200 lines — huge functions get truncated let content = if end_line - start_line > 200 { diff --git a/src/spawn.rs b/src/spawn.rs index 7940eaa..44c0d25 100644 --- a/src/spawn.rs +++ b/src/spawn.rs @@ -162,7 +162,7 @@ pub async fn spawn_agent( cancel_tx: Some(cancel_tx), }; - registry.lock().unwrap().insert(id.clone(), agent); + registry.lock().unwrap_or_else(|e| e.into_inner()).insert(id.clone(), agent); // Build config for the spawned agent let mut agent_config = config.clone(); @@ -301,7 +301,7 @@ async fn run_spawned_agent( /// List all agents with their status. pub fn list_agents(registry: &SpawnRegistry) -> String { - let reg = registry.lock().unwrap(); + let reg = registry.lock().unwrap_or_else(|e| e.into_inner()); if reg.is_empty() { return "No spawned agents.".to_string(); } @@ -327,7 +327,7 @@ pub fn list_agents(registry: &SpawnRegistry) -> String { /// Get the diff for a completed agent. pub fn review_agent(registry: &SpawnRegistry, id: &str) -> Result { - let reg = registry.lock().unwrap(); + let reg = registry.lock().unwrap_or_else(|e| e.into_inner()); let agent = find_agent(®, id)?; match &agent.status { @@ -370,7 +370,7 @@ pub fn review_agent(registry: &SpawnRegistry, id: &str) -> Result { /// Cancel a running agent. pub fn kill_agent(registry: &SpawnRegistry, id: &str) -> Result { - let mut reg = registry.lock().unwrap(); + let mut reg = registry.lock().unwrap_or_else(|e| e.into_inner()); let agent = find_agent_mut(&mut reg, id)?; if agent.status != SpawnStatus::Running { @@ -396,7 +396,7 @@ pub fn kill_agent(registry: &SpawnRegistry, id: &str) -> Result { /// Merge a completed agent's worktree changes into the current branch. pub async fn merge_agent(registry: &SpawnRegistry, id: &str, main_cwd: &PathBuf) -> Result { let (branch, wt_path) = { - let reg = registry.lock().unwrap(); + let reg = registry.lock().unwrap_or_else(|e| e.into_inner()); let agent = find_agent(®, id)?; if agent.status != SpawnStatus::Completed { anyhow::bail!( @@ -424,7 +424,7 @@ pub async fn merge_agent(registry: &SpawnRegistry, id: &str, main_cwd: &PathBuf) .await?; let commit_msg = format!("spawn: {}", { - let reg = registry.lock().unwrap(); + let reg = registry.lock().unwrap_or_else(|e| e.into_inner()); reg.get(id) .map(|a| a.description.clone()) .unwrap_or_default() @@ -490,7 +490,7 @@ pub async fn discard_agent( main_cwd: &PathBuf, ) -> Result { let (branch, wt_path, desc) = { - let reg = registry.lock().unwrap(); + let reg = registry.lock().unwrap_or_else(|e| e.into_inner()); let agent = find_agent(®, id)?; if agent.status == SpawnStatus::Running { anyhow::bail!( diff --git a/src/tools/cron.rs b/src/tools/cron.rs index 0e6faff..a04b317 100644 --- a/src/tools/cron.rs +++ b/src/tools/cron.rs @@ -185,7 +185,7 @@ impl Tool for CronCreateTool { }; { - let mut jobs = self.store.lock().unwrap(); + let mut jobs = self.store.lock().unwrap_or_else(|e| e.into_inner()); jobs.push(job); save_jobs(&jobs)?; } @@ -231,7 +231,7 @@ impl Tool for CronDeleteTool { async fn execute(&self, input: serde_json::Value, _ctx: &ToolContext) -> Result { let input: DeleteInput = serde_json::from_value(input)?; - let mut jobs = self.store.lock().unwrap(); + let mut jobs = self.store.lock().unwrap_or_else(|e| e.into_inner()); let before = jobs.len(); jobs.retain(|j| j.id != input.id); @@ -271,7 +271,7 @@ impl Tool for CronListTool { } async fn execute(&self, _input: serde_json::Value, _ctx: &ToolContext) -> Result { - let jobs = self.store.lock().unwrap().clone(); + let jobs = self.store.lock().unwrap_or_else(|e| e.into_inner()).clone(); if jobs.is_empty() { return Ok(ToolOutput::success("No cron jobs scheduled.")); diff --git a/src/tools/file_read.rs b/src/tools/file_read.rs index e683c2c..65d1aee 100644 --- a/src/tools/file_read.rs +++ b/src/tools/file_read.rs @@ -98,7 +98,7 @@ impl Tool for FileReadTool { let mut hasher = std::collections::hash_map::DefaultHasher::new(); content.hash(&mut hasher); let hash = hasher.finish(); - let mut guard = cache.lock().unwrap(); + let mut guard = cache.lock().unwrap_or_else(|e| e.into_inner()); if guard.get(&path) == Some(&hash) { return Ok(ToolOutput::success(format!( "(unchanged since last read: {})", diff --git a/src/tools/tasks.rs b/src/tools/tasks.rs index 082eb5f..89f1110 100644 --- a/src/tools/tasks.rs +++ b/src/tools/tasks.rs @@ -84,7 +84,7 @@ impl Tool for TaskCreateTool { output: None, active_form: input.active_form, }; - self.registry.lock().unwrap().insert(id.clone(), task); + self.registry.lock().unwrap_or_else(|e| e.into_inner()).insert(id.clone(), task); Ok(ToolOutput::success( json!({ "task": { "id": id } }).to_string(), )) @@ -124,7 +124,7 @@ impl Tool for TaskGetTool { async fn execute(&self, input: serde_json::Value, _ctx: &ToolContext) -> Result { let input: GetInput = serde_json::from_value(input)?; - let registry = self.registry.lock().unwrap(); + let registry = self.registry.lock().unwrap_or_else(|e| e.into_inner()); match registry.get(&input.task_id) { Some(task) => Ok(ToolOutput::success(serde_json::to_string(task)?)), None => Ok(ToolOutput::error(format!( @@ -156,7 +156,7 @@ impl Tool for TaskListTool { } async fn execute(&self, _input: serde_json::Value, _ctx: &ToolContext) -> Result { - let registry = self.registry.lock().unwrap(); + let registry = self.registry.lock().unwrap_or_else(|e| e.into_inner()); if registry.is_empty() { return Ok(ToolOutput::success("No tasks.")); } @@ -207,7 +207,7 @@ impl Tool for TaskUpdateTool { async fn execute(&self, input: serde_json::Value, _ctx: &ToolContext) -> Result { let input: UpdateInput = serde_json::from_value(input)?; - let mut registry = self.registry.lock().unwrap(); + let mut registry = self.registry.lock().unwrap_or_else(|e| e.into_inner()); match registry.get_mut(&input.task_id) { Some(task) => { if let Some(s) = input.status { @@ -262,7 +262,7 @@ impl Tool for TaskStopTool { async fn execute(&self, input: serde_json::Value, _ctx: &ToolContext) -> Result { let input: StopInput = serde_json::from_value(input)?; - let mut registry = self.registry.lock().unwrap(); + let mut registry = self.registry.lock().unwrap_or_else(|e| e.into_inner()); match registry.get_mut(&input.task_id) { Some(task) => { task.status = TaskStatus::Stopped; @@ -324,7 +324,7 @@ impl Tool for TaskOutputTool { async fn execute(&self, input: serde_json::Value, _ctx: &ToolContext) -> Result { let input: OutputInput = serde_json::from_value(input)?; - let mut registry = self.registry.lock().unwrap(); + let mut registry = self.registry.lock().unwrap_or_else(|e| e.into_inner()); match registry.get_mut(&input.task_id) { Some(task) => { task.output = Some(input.output); diff --git a/src/tools/worktree.rs b/src/tools/worktree.rs index 9819f21..75e8b2c 100644 --- a/src/tools/worktree.rs +++ b/src/tools/worktree.rs @@ -64,7 +64,7 @@ impl Tool for EnterWorktreeTool { let input: EnterInput = serde_json::from_value(input)?; // Must not already be in a worktree - if self.state.lock().unwrap().is_some() { + if self.state.lock().unwrap_or_else(|e| e.into_inner()).is_some() { return Ok(ToolOutput::error( "Already in a worktree session. Use ExitWorktree first.", )); @@ -144,7 +144,7 @@ impl Tool for EnterWorktreeTool { branch: slug.clone(), original_cwd: ctx.cwd.clone(), }; - *self.state.lock().unwrap() = Some(session); + *self.state.lock().unwrap_or_else(|e| e.into_inner()) = Some(session); Ok(ToolOutput::success( json!({ @@ -179,7 +179,7 @@ impl Tool for ExitWorktreeTool { } async fn execute(&self, _input: serde_json::Value, _ctx: &ToolContext) -> Result { - let session = self.state.lock().unwrap().take(); + let session = self.state.lock().unwrap_or_else(|e| e.into_inner()).take(); match session { None => Ok(ToolOutput::error("Not currently in a worktree session.")), diff --git a/src/tui/app.rs b/src/tui/app.rs index 4391721..31ef975 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -1028,11 +1028,15 @@ impl App { if self.input_history.is_empty() { return; } - if self.history_idx.is_none() { - self.saved_input = self.input.clone(); - self.history_idx = Some(self.input_history.len() - 1); - } else if self.history_idx.unwrap() > 0 { - self.history_idx = Some(self.history_idx.unwrap() - 1); + match self.history_idx { + None => { + self.saved_input = self.input.clone(); + self.history_idx = Some(self.input_history.len() - 1); + } + Some(i) if i > 0 => { + self.history_idx = Some(i - 1); + } + _ => {} } if let Some(i) = self.history_idx { self.input = self.input_history[i].chars().collect(); @@ -1043,10 +1047,9 @@ impl App { pub fn history_down(&mut self) { if let Some(i) = self.history_idx { if i + 1 < self.input_history.len() { - self.history_idx = Some(i + 1); - self.input = self.input_history[self.history_idx.unwrap()] - .chars() - .collect(); + let next = i + 1; + self.history_idx = Some(next); + self.input = self.input_history[next].chars().collect(); } else { self.history_idx = None; self.input = self.saved_input.clone(); diff --git a/src/tui/render.rs b/src/tui/render.rs index 13ce70d..ed8b6c5 100644 --- a/src/tui/render.rs +++ b/src/tui/render.rs @@ -304,7 +304,11 @@ fn draw_banner_right(f: &mut Frame, area: Rect, app: &App, tc: ThemeColors) { // Truncate preview to fit: max_w minus the "◆ [id] — " prefix (~16 chars) let avail = max_w.saturating_sub(16); if preview.len() > avail { - format!("{}…", &preview[..avail.saturating_sub(1).max(1)]) + let mut end = avail.saturating_sub(1).max(1); + while end > 0 && !preview.is_char_boundary(end) { + end -= 1; + } + format!("{}…", &preview[..end]) } else { preview.clone() } From 53c48f16cf83ec0faa295bbba3479eaf82ed92d1 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Wed, 22 Apr 2026 21:30:30 -0700 Subject: [PATCH 2/2] feat(browse): wire Step/ApprovalNeeded progress, voice milestones, register browse_done Finish the browse agent scaffolding that was shipping with unused warnings: - Register BrowseDoneTool so the model has a tool-call path to signal completion. Previously only the text-sentinel fallback could fire despite the tool type being defined. - Add StepEmitterMiddleware (runs last in the chain so denied calls are not reported) that emits BrowseProgress::Step {n, action, target} for every allowed tool invocation. Step counter now owned by the emitter; approval_gate reads counter+1 for the prompt's step field. - Bridge ApprovalPrompt -> BrowseProgress::ApprovalNeeded via an internal channel and a forwarding task, so TUI and SDK can surface approval events through the same progress stream. - Wire BrowseRequest.voice to speak_browse_milestone at Start (goal), GateTrip (approval reason), and End (final summary). When voice is on, the approval gate races a 60s voice listener against the keyboard reply so users can say "yes" to approve. - Delete genuinely dead scaffolding: - MiddlewareVerdict::RequireConfirmation (never emitted; approval_gate resolves internally and returns Allow/Deny) - LoopDetector::window_len (unused observer for internal state) - fingerprint_action (unused public helper; hash_string is the internal equivalent used by record_action) - Corresponding test cases updated to the new surface. Build is warning-free; 300+ tests pass. Co-Authored-By: Arch Linux --- src/browser/approval_gate.rs | 43 +++++++++--- src/browser/browse_loop.rs | 125 +++++++++++++++++++++++++++++++++-- src/browser/loop_detector.rs | 9 --- src/browser/middleware.rs | 5 +- src/query_engine.rs | 12 ---- src/tools/mod.rs | 1 + tests/browse_integration.rs | 6 +- tests/loop_detector_tests.rs | 52 ++++----------- tests/middleware_smoke.rs | 31 +-------- 9 files changed, 168 insertions(+), 116 deletions(-) diff --git a/src/browser/approval_gate.rs b/src/browser/approval_gate.rs index eb7ef27..0feaf20 100644 --- a/src/browser/approval_gate.rs +++ b/src/browser/approval_gate.rs @@ -230,6 +230,8 @@ pub struct ApprovalGateMiddleware { denial_counts: Mutex>, /// Set when the same action is denied twice — triggers session termination. user_denied: AtomicBool, + /// When true, also listen for a spoken "yes/approve" alongside the keyboard reply. + voice: bool, } impl ApprovalGateMiddleware { @@ -239,6 +241,7 @@ impl ApprovalGateMiddleware { current_url: Arc>, approval_tx: mpsc::Sender, step_counter: Arc, + voice: bool, ) -> Self { Self { gate, @@ -248,6 +251,7 @@ impl ApprovalGateMiddleware { step_counter, denial_counts: Mutex::new(HashMap::new()), user_denied: AtomicBool::new(false), + voice, } } @@ -313,7 +317,8 @@ impl ToolMiddleware for ApprovalGateMiddleware { MiddlewareVerdict::Allow } GateVerdict::RequireConfirmation { reason, .. } => { - let step = self.step_counter.load(Ordering::Relaxed); + // +1 because the step_emitter middleware increments after the gate runs. + let step = self.step_counter.load(Ordering::Relaxed) + 1; let (tx, rx) = oneshot::channel(); let prompt = ApprovalPrompt { step, @@ -330,14 +335,34 @@ impl ToolMiddleware for ApprovalGateMiddleware { }; } use tokio::time::{timeout, Duration}; - match timeout(Duration::from_secs(60), rx).await { - Ok(Ok(true)) => { + // If voice is on, race the keyboard reply against a voice-approval + // listener. Whichever resolves first wins. Voice only contributes + // an Approve vote (false/timeout is ignored unless no keyboard reply + // arrives either). + let approved_opt: Option = if self.voice { + tokio::select! { + kb = timeout(Duration::from_secs(60), rx) => match kb { + Ok(Ok(b)) => Some(b), + _ => None, + }, + voice_yes = crate::voice::await_voice_approval(60) => { + if voice_yes { Some(true) } else { None } + } + } + } else { + match timeout(Duration::from_secs(60), rx).await { + Ok(Ok(b)) => Some(b), + _ => None, + } + }; + match approved_opt { + Some(true) => { // Approved — clear denial counter for this action. let key = format!("{tool_name}:{target_text}"); self.denial_counts.lock().unwrap_or_else(|e| e.into_inner()).remove(&key); MiddlewareVerdict::Allow } - Ok(Ok(false)) => { + Some(false) => { // User explicitly denied — increment counter; terminate after 2 denials. let key = format!("{tool_name}:{target_text}"); let count = { @@ -355,11 +380,8 @@ impl ToolMiddleware for ApprovalGateMiddleware { } MiddlewareVerdict::Deny { reason } } - Ok(Err(_)) => MiddlewareVerdict::Deny { - reason: "approval channel dropped".into(), - }, - Err(_) => MiddlewareVerdict::Deny { - reason: "approval timed out (60s)".into(), + None => MiddlewareVerdict::Deny { + reason: "approval timed out or channel dropped".into(), }, } } @@ -367,7 +389,6 @@ impl ToolMiddleware for ApprovalGateMiddleware { } async fn after_tool(&self, _tool_name: &str, _output: &str) { - // Increment step counter after each tool execution. - self.step_counter.fetch_add(1, Ordering::Relaxed); + // Step counting is owned by StepEmitterMiddleware (runs after this middleware). } } diff --git a/src/browser/browse_loop.rs b/src/browser/browse_loop.rs index f0cab7e..26fa5be 100644 --- a/src/browser/browse_loop.rs +++ b/src/browser/browse_loop.rs @@ -1,4 +1,5 @@ use anyhow::Result; +use async_trait::async_trait; use serde::{Deserialize, Serialize}; use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::sync::Arc; @@ -6,6 +7,7 @@ use tokio::sync::mpsc; use crate::browser::approval_gate::{ApprovalGate, ApprovalGateMiddleware, ApprovalPrompt}; use crate::browser::loop_detector::LoopDetectorMiddleware; +use crate::browser::middleware::{MiddlewareVerdict, ToolMiddleware}; use crate::config::Config; use crate::query_engine::QueryEngine; use crate::tools::DynTool; @@ -110,6 +112,45 @@ fn parse_browse_done(text: &str) -> Option<(bool, String)> { Some((achieved, summary)) } +/// Extract a human-readable "target" from tool input for Step events. +/// Prefers `url` → `ref` → `selector` → `key` → empty string. +fn extract_target(input: &serde_json::Value) -> String { + for key in ["url", "ref", "selector", "key"] { + if let Some(s) = input.get(key).and_then(|v| v.as_str()) { + if !s.is_empty() { + return s.to_string(); + } + } + } + String::new() +} + +/// Middleware that emits `BrowseProgress::Step` for each allowed tool call. +/// Placed last in the chain so denied calls (by gate or loop detector) are not +/// reported as executed steps. +struct StepEmitterMiddleware { + progress_tx: mpsc::Sender, + counter: Arc, +} + +#[async_trait] +impl ToolMiddleware for StepEmitterMiddleware { + async fn before_tool(&self, tool_name: &str, input: &serde_json::Value) -> MiddlewareVerdict { + let n = self.counter.fetch_add(1, Ordering::Relaxed) + 1; + let _ = self + .progress_tx + .send(BrowseProgress::Step { + n, + action: tool_name.to_string(), + target: extract_target(input), + }) + .await; + MiddlewareVerdict::Allow + } + + async fn after_tool(&self, _tool_name: &str, _output: &str) {} +} + /// Orchestrate an autonomous browser agent run. pub async fn run_browse( req: BrowseRequest, @@ -120,35 +161,90 @@ pub async fn run_browse( approval_tx: mpsc::Sender, cancel: Arc, ) -> Result { - // 1. Emit Started event. + // 1. Emit Started event + speak the goal if voice is enabled. let _ = progress_tx .send(BrowseProgress::Started { goal: req.goal.clone(), max_steps: req.max_steps, }) .await; + if req.voice { + let goal = req.goal.clone(); + tokio::spawn(async move { + crate::voice::speak_browse_milestone( + crate::voice::BrowseMilestone::Start, + &goal, + ) + .await; + }); + } // 2. Shared step counter for the approval prompt. let step_counter = Arc::new(AtomicU32::new(0)); // 3. Build the approval gate from config patterns. + // The gate sends prompts to an internal channel; a bridge task mirrors each + // prompt as BrowseProgress::ApprovalNeeded, then forwards it to the caller. let gate = ApprovalGate::with_user_patterns(config.browse_approval_patterns.clone()); + let (internal_approval_tx, mut internal_approval_rx) = mpsc::channel::(16); let gate_mw = Arc::new(ApprovalGateMiddleware::new( gate, req.policy, current_url.clone(), - approval_tx, + internal_approval_tx, step_counter.clone(), + req.voice, )); + // Bridge: internal approvals → progress event + external approval channel. + let approval_bridge_progress_tx = progress_tx.clone(); + let voice_on_gate = req.voice; + let approval_bridge_handle = tokio::spawn(async move { + while let Some(prompt) = internal_approval_rx.recv().await { + let _ = approval_bridge_progress_tx + .send(BrowseProgress::ApprovalNeeded { + step: prompt.step, + action: prompt.tool_name.clone(), + target_text: prompt.target_text.clone(), + url: prompt.url.clone(), + reason: prompt.reason.clone(), + }) + .await; + if voice_on_gate { + let phrase = format!( + "Approval needed for {} — {}", + prompt.tool_name, prompt.reason + ); + tokio::spawn(async move { + crate::voice::speak_browse_milestone( + crate::voice::BrowseMilestone::GateTrip, + &phrase, + ) + .await; + }); + } + if approval_tx.send(prompt).await.is_err() { + // Caller dropped the approval channel — stop bridging. + break; + } + } + }); + // 4. Build the loop detector middleware. let (nudge_tx, mut nudge_rx) = mpsc::channel::(16); let loop_mw = Arc::new(LoopDetectorMiddleware::new(nudge_tx)); - // 5. Assemble middleware chain (keep Arc refs for post-run inspection). + // 5. Build the step-emitter middleware (runs last — only fires for allowed calls). + let step_emitter = Arc::new(StepEmitterMiddleware { + progress_tx: progress_tx.clone(), + counter: step_counter.clone(), + }); + + // 6. Assemble middleware chain (keep Arc refs for post-run inspection). let middlewares: crate::browser::middleware::MiddlewareChain = vec![ - gate_mw.clone() as Arc, - loop_mw.clone() as Arc, + gate_mw.clone() as Arc, + loop_mw.clone() as Arc, + step_emitter as Arc, ]; // 6. Build browse-specific system prompt. @@ -179,8 +275,9 @@ pub async fn run_browse( // 11. Check for early cancellation before starting the loop. if cancel.load(Ordering::SeqCst) { - drop(engine); // drop engine (and its middleware chain) to shut down nudge_tx + drop(engine); // drop engine (and its middleware chain) to shut down channels let _ = nudge_handle.await; + let _ = approval_bridge_handle.await; let final_url = { let url = current_url.lock().await; if url.is_empty() { None } else { Some(url.clone()) } @@ -317,10 +414,24 @@ pub async fn run_browse( } }; - // 14. Emit Completed event. + // 14. Emit Completed event + speak the final summary if voice is enabled. let _ = progress_tx .send(BrowseProgress::Completed(result.clone())) .await; + if req.voice { + let phrase = if result.achieved { + format!("Done. {}", result.summary) + } else { + format!("Stopped. {}", result.summary) + }; + tokio::spawn(async move { + crate::voice::speak_browse_milestone( + crate::voice::BrowseMilestone::End, + &phrase, + ) + .await; + }); + } Ok(result) } diff --git a/src/browser/loop_detector.rs b/src/browser/loop_detector.rs index b2d7e00..40d236d 100644 --- a/src/browser/loop_detector.rs +++ b/src/browser/loop_detector.rs @@ -76,10 +76,6 @@ impl LoopDetector { self.window.clear(); self.nudge_level = 0; } - - pub fn window_len(&self) -> usize { - self.window.len() - } } impl Default for LoopDetector { @@ -88,11 +84,6 @@ impl Default for LoopDetector { } } -/// Public helper: SHA-256 of "{action_type}:{target}". -pub fn fingerprint_action(action_type: &str, target: &str, _extra: &str) -> String { - hash_string(&format!("{action_type}:{target}")) -} - fn hash_string(s: &str) -> String { let mut hasher = Sha256::new(); hasher.update(s.as_bytes()); diff --git a/src/browser/middleware.rs b/src/browser/middleware.rs index 7805214..32c4a07 100644 --- a/src/browser/middleware.rs +++ b/src/browser/middleware.rs @@ -14,15 +14,12 @@ pub enum MiddlewareVerdict { Allow, /// Block the tool with an error reason. Deny { reason: String }, - /// Request confirmation before proceeding. - /// Treated as Deny until the approval gate resolves it internally. - RequireConfirmation { reason: String, detail: String }, } /// Extension point invoked before and after every tool execution. #[async_trait] pub trait ToolMiddleware: Send + Sync { - /// Called before a tool runs. Return `Deny` or `RequireConfirmation` to block. + /// Called before a tool runs. Return `Deny` to block. async fn before_tool(&self, tool_name: &str, input: &Value) -> MiddlewareVerdict; /// Called after a tool runs with its output text. diff --git a/src/query_engine.rs b/src/query_engine.rs index 2ab40f7..0d604db 100644 --- a/src/query_engine.rs +++ b/src/query_engine.rs @@ -439,18 +439,6 @@ impl QueryEngine { middleware_denied = true; break; } - MiddlewareVerdict::RequireConfirmation { reason, .. } => { - // Treat as Deny until the approval gate resolves internally. - results.push(ContentBlock::ToolResult { - tool_use_id: id.clone(), - content: vec![ToolResultContent::text(format!( - "Middleware requires confirmation: {reason}" - ))], - is_error: Some(true), - }); - middleware_denied = true; - break; - } } } if middleware_denied { diff --git a/src/tools/mod.rs b/src/tools/mod.rs index dc8de14..00fab66 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -487,6 +487,7 @@ pub fn all_tools_with_state(config: &crate::config::Config) -> (Vec, Sh session: browser_session.clone(), default_timeout_ms: config.browser_timeout_ms, })); + tools.push(Arc::new(browser_tools::BrowseDoneTool)); Some(browser_session) } else { None diff --git a/tests/browse_integration.rs b/tests/browse_integration.rs index 1b62d3f..7a5c7db 100644 --- a/tests/browse_integration.rs +++ b/tests/browse_integration.rs @@ -114,7 +114,7 @@ async fn approval_middleware_allows_read_tools() { let (approval_tx, _) = tokio::sync::mpsc::channel(4); let step = Arc::new(AtomicU32::new(0)); - let mw = ApprovalGateMiddleware::new(gate, BrowsePolicy::Pattern, current_url, approval_tx, step); + let mw = ApprovalGateMiddleware::new(gate, BrowsePolicy::Pattern, current_url, approval_tx, step, false); let verdict = mw.before_tool("browser_navigate", &json!({"url": "https://example.com"})).await; assert!(matches!(verdict, MiddlewareVerdict::Allow)); @@ -127,7 +127,7 @@ async fn approval_middleware_yolo_allows_everything() { let (approval_tx, _) = tokio::sync::mpsc::channel(4); let step = Arc::new(AtomicU32::new(0)); - let mw = ApprovalGateMiddleware::new(gate, BrowsePolicy::Yolo, current_url, approval_tx, step); + let mw = ApprovalGateMiddleware::new(gate, BrowsePolicy::Yolo, current_url, approval_tx, step, false); let verdict = mw.before_tool("browser_click", &json!({"ref": "@e1"})).await; assert!(matches!(verdict, MiddlewareVerdict::Allow)); @@ -142,7 +142,7 @@ async fn approval_middleware_denies_on_dropped_channel() { drop(approval_rx); - let mw = ApprovalGateMiddleware::new(gate, BrowsePolicy::Pattern, current_url, approval_tx, step); + let mw = ApprovalGateMiddleware::new(gate, BrowsePolicy::Pattern, current_url, approval_tx, step, false); let verdict = mw.before_tool("browser_click", &json!({"ref": "@e1"})).await; assert!(matches!(verdict, MiddlewareVerdict::Deny { .. })); diff --git a/tests/loop_detector_tests.rs b/tests/loop_detector_tests.rs index e72ab6d..3c8f906 100644 --- a/tests/loop_detector_tests.rs +++ b/tests/loop_detector_tests.rs @@ -1,32 +1,6 @@ -use rustyclaw::browser::loop_detector::{fingerprint_action, LoopDetector}; +use rustyclaw::browser::loop_detector::LoopDetector; -// 1. Same inputs → same hash -#[test] -fn fingerprint_equality_same_inputs() { - let h1 = fingerprint_action("click", "#submit", ""); - let h2 = fingerprint_action("click", "#submit", ""); - assert_eq!(h1, h2); -} - -// 2. Different target → different hash -#[test] -fn fingerprint_differs_on_target() { - let h1 = fingerprint_action("click", "#submit", ""); - let h2 = fingerprint_action("click", "#cancel", ""); - assert_ne!(h1, h2); -} - -// 3. 11 records → window capped at 10 -#[test] -fn window_prunes_oldest_at_eleven() { - let mut d = LoopDetector::new(); - for i in 0..11 { - d.record_action("click", &format!("#{i}"), "page"); - } - assert_eq!(d.window_len(), 10); -} - -// 4. Three identical actions → level-1 nudge ("different approach") +// 1. Three identical actions → level-1 nudge ("different approach") #[test] fn three_identical_trips_level_one_nudge() { let mut d = LoopDetector::new(); @@ -37,17 +11,14 @@ fn three_identical_trips_level_one_nudge() { assert!(nudge.contains("different approach"), "got: {nudge}"); } -// 5. After level 1, three more identical → level-2 nudge ("multiple times") +// 2. After level 1, three more identical → level-2 nudge ("multiple times") #[test] fn three_more_identical_escalate_to_level_two() { let mut d = LoopDetector::new(); - // Trigger level 1 for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } d.check_stagnation(); - // Trigger level 2 — window already has 3 identical, check_stagnation consumed level 0. - // We need to push 3 more identical entries so tail is still identical. for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } @@ -55,18 +26,18 @@ fn three_more_identical_escalate_to_level_two() { assert!(nudge.contains("multiple times"), "got: {nudge}"); } -// 6. Third check → terminal "Stopping" nudge +// 3. Third check → terminal "Stopping" nudge #[test] fn level_three_is_terminal_stop() { let mut d = LoopDetector::new(); for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } - d.check_stagnation(); // level 0 → emit NUDGES[0], nudge_level becomes 1 + d.check_stagnation(); for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } - d.check_stagnation(); // level 1 → emit NUDGES[1], nudge_level becomes 2 + d.check_stagnation(); for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } @@ -74,17 +45,18 @@ fn level_three_is_terminal_stop() { assert!(nudge.contains("Stopping"), "got: {nudge}"); } -// 7. reset() clears window and resets nudge level +// 4. reset() clears window and resets nudge level #[test] fn reset_clears_window_and_nudge_level() { let mut d = LoopDetector::new(); for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } - d.check_stagnation(); // consumes level 0 + d.check_stagnation(); d.reset(); - assert_eq!(d.window_len(), 0); - // After reset, 3 identical actions should produce level-1 nudge again + // After reset, no stagnation from zero entries. + assert!(d.check_stagnation().is_none()); + // And 3 identical actions should produce level-1 nudge again for _ in 0..3 { d.record_action("click", "#btn", "same page text"); } @@ -92,7 +64,7 @@ fn reset_clears_window_and_nudge_level() { assert!(nudge.contains("different approach"), "got: {nudge}"); } -// 8. Same action but different page_text each time → no stagnation +// 5. Same action but different page_text each time → no stagnation #[test] fn page_change_breaks_stagnation() { let mut d = LoopDetector::new(); diff --git a/tests/middleware_smoke.rs b/tests/middleware_smoke.rs index f640b1c..fff2410 100644 --- a/tests/middleware_smoke.rs +++ b/tests/middleware_smoke.rs @@ -81,35 +81,6 @@ async fn denier_middleware_returns_deny() { } } -// ── RequireConfirmation ────────────────────────────────────────────── - -struct ConfirmationRequester; - -#[async_trait] -impl ToolMiddleware for ConfirmationRequester { - async fn before_tool(&self, tool_name: &str, _input: &Value) -> MiddlewareVerdict { - MiddlewareVerdict::RequireConfirmation { - reason: format!("{tool_name} needs approval"), - detail: "destructive action".into(), - } - } - - async fn after_tool(&self, _tool_name: &str, _output: &str) {} -} - -#[tokio::test] -async fn require_confirmation_variant() { - let mw = ConfirmationRequester; - let verdict = mw.before_tool("bash", &serde_json::json!({})).await; - match verdict { - MiddlewareVerdict::RequireConfirmation { reason, detail } => { - assert_eq!(reason, "bash needs approval"); - assert_eq!(detail, "destructive action"); - } - other => panic!("Expected RequireConfirmation, got {other:?}"), - } -} - // ── Chain ordering ─────────────────────────────────────────────────── #[tokio::test] @@ -128,7 +99,7 @@ async fn middleware_chain_short_circuits_on_deny() { for mw in &chain { match mw.before_tool("bash", &input).await { MiddlewareVerdict::Allow => {} - MiddlewareVerdict::Deny { .. } | MiddlewareVerdict::RequireConfirmation { .. } => { + MiddlewareVerdict::Deny { .. } => { denied = true; break; }