From 0d0a514c31cbb57026aab1839453b0592a4e78fd Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Tue, 3 Feb 2026 05:27:42 -0600 Subject: [PATCH] fix: batch config wiring quick wins (#262-#266) Five post-PR-261 fixes to close config wiring gaps: - Deprecate with_provider/with_provider_and_path bypass constructors (#262) - Wire on_compaction callback for TUI status feedback (#264) - Extract build_system_prompt_from_config for -P mode parity (#266) - Wire dangerousPatterns config through to tool confirmation (#263) - Cache running_char_count to avoid re-serializing JSON per iteration (#265) Co-Authored-By: Claude Opus 4.5 --- codi-rs/src/agent/mod.rs | 138 +++++++++++++++++++------ codi-rs/src/agent/types.rs | 25 +++++ codi-rs/src/main.rs | 5 +- codi-rs/src/orchestrate/child_agent.rs | 1 + codi-rs/src/tui/app.rs | 99 +++++++++++++++--- codi-rs/src/tui/mod.rs | 2 +- 6 files changed, 220 insertions(+), 50 deletions(-) diff --git a/codi-rs/src/agent/mod.rs b/codi-rs/src/agent/mod.rs index 006fde5..ed123c2 100644 --- a/codi-rs/src/agent/mod.rs +++ b/codi-rs/src/agent/mod.rs @@ -135,20 +135,16 @@ impl Agent { /// Estimate the current token count of all messages. /// Uses the standard approximation of ~4 characters per token. + /// Leverages `running_char_count` to avoid re-serializing every message each iteration. fn estimate_tokens(&self) -> usize { - let mut total_chars: usize = 0; + let mut total_chars: usize = self.state.running_char_count; - // Count system prompt + // Add system prompt + summary (not cached — these are cheap to measure) total_chars += self.system_prompt.len(); if let Some(ref summary) = self.state.conversation_summary { total_chars += summary.len(); } - // Count all messages - for msg in &self.state.messages { - total_chars += self.message_char_count(msg); - } - total_chars / 4 } @@ -239,6 +235,11 @@ impl Agent { self.state.conversation_summary = Some(new_summary); } + // Recalculate running_char_count from remaining messages + self.state.running_char_count = self.state.messages.iter() + .map(|m| self.message_char_count(m)) + .sum(); + tracing::info!( "Context compacted: removed {} messages, {} remaining", split_at, @@ -251,24 +252,41 @@ impl Agent { } } - /// Check if a tool call should be confirmed. - fn should_confirm(&self, tool_name: &str) -> bool { - self.config.requires_confirmation(tool_name) && self.callbacks.on_confirm.is_some() - } - - /// Confirm a tool call with the user. - fn confirm_tool(&self, tool_call: &ToolCall) -> ConfirmationResult { - if let Some(ref on_confirm) = self.callbacks.on_confirm { - let confirmation = ToolConfirmation { - tool_name: tool_call.name.clone(), - input: tool_call.input.clone(), - is_dangerous: DESTRUCTIVE_TOOLS.contains(&tool_call.name.as_str()), - danger_reason: None, // TODO: Add danger detection - }; - on_confirm(confirmation) + /// Check whether a tool call needs confirmation and, if so, ask the user. + /// + /// Returns `None` when no confirmation is needed (tool is auto-approved and + /// no dangerous pattern matches). Otherwise returns the user's decision. + /// Serializes the input only once to avoid redundant work. + fn maybe_confirm(&self, tool_call: &ToolCall) -> Option { + let on_confirm = self.callbacks.on_confirm.as_ref()?; + + let is_builtin_dangerous = DESTRUCTIVE_TOOLS.contains(&tool_call.name.as_str()); + let needs_builtin_confirm = is_builtin_dangerous + && !self.config.should_auto_approve(&tool_call.name); + + // Serialize input once and check dangerous patterns + let pattern_match = if !self.config.dangerous_patterns.is_empty() { + let input_str = tool_call.input.to_string(); + self.config.matches_dangerous_pattern(&input_str) } else { - ConfirmationResult::Approve + None + }; + + // If neither builtin-destructive nor pattern-matched, no confirmation needed + if !needs_builtin_confirm && pattern_match.is_none() { + return None; } + + let is_dangerous = is_builtin_dangerous || pattern_match.is_some(); + let danger_reason = pattern_match.map(|p| format!("Matches dangerous pattern: {}", p)); + + let confirmation = ToolConfirmation { + tool_name: tool_call.name.clone(), + input: tool_call.input.clone(), + is_dangerous, + danger_reason, + }; + Some(on_confirm(confirmation)) } /// Execute a single tool call. @@ -325,9 +343,9 @@ impl Agent { let mut has_error = false; for tool_call in tool_calls { - // Check if confirmation is needed - if self.should_confirm(&tool_call.name) { - match self.confirm_tool(tool_call) { + // Check if confirmation is needed, and if so, get the user's decision + if let Some(decision) = self.maybe_confirm(tool_call) { + match decision { ConfirmationResult::Approve => { // Continue to execute } @@ -387,10 +405,12 @@ impl Agent { .map(|r| ContentBlock::tool_result(&r.tool_use_id, &r.content, r.is_error.unwrap_or(false))) .collect(); - self.state.messages.push(Message { + let msg = Message { role: Role::User, content: crate::types::MessageContent::Blocks(content), - }); + }; + self.state.running_char_count += self.message_char_count(&msg); + self.state.messages.push(msg); } /// The main agentic loop. @@ -405,7 +425,9 @@ impl Agent { let mut turn_stats = TurnStats::default(); // Add user message to history - self.state.messages.push(Message::user(user_message)); + let user_msg = Message::user(user_message); + self.state.running_char_count += self.message_char_count(&user_msg); + self.state.messages.push(user_msg); // Reset iteration state self.state.current_iteration = 0; @@ -485,10 +507,12 @@ impl Agent { } if !assistant_blocks.is_empty() { - self.state.messages.push(Message { + let assistant_msg = Message { role: Role::Assistant, content: crate::types::MessageContent::Blocks(assistant_blocks), - }); + }; + self.state.running_char_count += self.message_char_count(&assistant_msg); + self.state.messages.push(assistant_msg); } // If no tool calls, we're done @@ -637,4 +661,56 @@ mod tests { assert!(result.ends_with("...")); assert!(!result.contains("world")); } + + #[test] + fn test_agent_state_default_running_char_count() { + let state = AgentState::default(); + assert_eq!(state.running_char_count, 0); + } + + #[test] + fn test_dangerous_pattern_match() { + let mut config = AgentConfig::default(); + config.dangerous_patterns = vec![ + r"rm\s+-rf".to_string(), + r"sudo\s+".to_string(), + ]; + + assert_eq!( + config.matches_dangerous_pattern("rm -rf /"), + Some(r"rm\s+-rf".to_string()) + ); + assert_eq!( + config.matches_dangerous_pattern("sudo apt install"), + Some(r"sudo\s+".to_string()) + ); + assert_eq!( + config.matches_dangerous_pattern("echo hello"), + None, + ); + } + + #[test] + fn test_dangerous_pattern_empty() { + let config = AgentConfig::default(); + assert!(config.dangerous_patterns.is_empty()); + assert_eq!(config.matches_dangerous_pattern("rm -rf /"), None); + } + + #[test] + fn test_dangerous_pattern_invalid_regex_skipped() { + let mut config = AgentConfig::default(); + config.dangerous_patterns = vec![ + "[invalid".to_string(), // bad regex + r"rm\s+-rf".to_string(), // valid + ]; + + // Should skip the invalid pattern gracefully and still match the valid one + assert_eq!( + config.matches_dangerous_pattern("rm -rf /"), + Some(r"rm\s+-rf".to_string()) + ); + // Invalid pattern should not cause a panic + assert_eq!(config.matches_dangerous_pattern("hello"), None); + } } diff --git a/codi-rs/src/agent/types.rs b/codi-rs/src/agent/types.rs index bfb3796..1f4c27a 100644 --- a/codi-rs/src/agent/types.rs +++ b/codi-rs/src/agent/types.rs @@ -130,6 +130,8 @@ pub struct AgentConfig { pub auto_approve_all: bool, /// Auto-approve specific tools by name. pub auto_approve_tools: Vec, + /// Regex patterns that flag tool inputs as dangerous (from config `dangerousPatterns`). + pub dangerous_patterns: Vec, } impl Default for AgentConfig { @@ -143,6 +145,7 @@ impl Default for AgentConfig { extract_tools_from_text: true, auto_approve_all: false, auto_approve_tools: Vec::new(), + dangerous_patterns: Vec::new(), } } } @@ -166,6 +169,25 @@ impl AgentConfig { pub fn requires_confirmation(&self, tool_name: &str) -> bool { DESTRUCTIVE_TOOLS.contains(&tool_name) && !self.should_auto_approve(tool_name) } + + /// Check if any dangerous pattern matches the given input string. + /// Returns the first matching pattern, or `None` if no pattern matches. + /// Invalid regex patterns are silently skipped. + pub fn matches_dangerous_pattern(&self, input_str: &str) -> Option { + for pat in &self.dangerous_patterns { + match regex::Regex::new(pat) { + Ok(re) => { + if re.is_match(input_str) { + return Some(pat.clone()); + } + } + Err(e) => { + tracing::warn!("Invalid dangerous pattern '{}': {}", pat, e); + } + } + } + None + } } /// Options for creating an agent. @@ -193,6 +215,8 @@ pub struct AgentState { pub current_iteration: usize, /// Consecutive error count. pub consecutive_errors: usize, + /// Running character count across all messages (avoids re-serializing JSON each iteration). + pub running_char_count: usize, } impl Default for AgentState { @@ -202,6 +226,7 @@ impl Default for AgentState { conversation_summary: None, current_iteration: 0, consecutive_errors: 0, + running_char_count: 0, } } } diff --git a/codi-rs/src/main.rs b/codi-rs/src/main.rs index b4601ed..1911e38 100644 --- a/codi-rs/src/main.rs +++ b/codi-rs/src/main.rs @@ -12,7 +12,7 @@ use codi::agent::AgentConfig; use codi::config::{self, CliOptions}; use codi::providers::{create_provider_from_config, ProviderType}; use codi::tools::ToolRegistry; -use codi::tui::{App, run as run_tui}; +use codi::tui::{App, build_system_prompt_from_config, run as run_tui}; /// Codi version string. const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -459,13 +459,14 @@ async fn handle_prompt( extract_tools_from_text: config.extract_tools_from_text, auto_approve_all: auto_approve, auto_approve_tools: config.auto_approve.clone(), + dangerous_patterns: config.dangerous_patterns.clone(), }; // Create and run agent let mut agent = codi::agent::Agent::new(codi::agent::AgentOptions { provider, tool_registry: registry, - system_prompt: Some("You are Codi, a helpful AI coding assistant.".to_string()), + system_prompt: Some(build_system_prompt_from_config(Some(config))), config: agent_config, callbacks: codi::agent::AgentCallbacks::default(), }); diff --git a/codi-rs/src/orchestrate/child_agent.rs b/codi-rs/src/orchestrate/child_agent.rs index f6e3c5f..9af53c0 100644 --- a/codi-rs/src/orchestrate/child_agent.rs +++ b/codi-rs/src/orchestrate/child_agent.rs @@ -241,6 +241,7 @@ impl ChildAgent { extract_tools_from_text: true, auto_approve_all: false, auto_approve_tools: self.auto_approve.clone(), + dangerous_patterns: Vec::new(), }; let mut agent = Agent::new(AgentOptions { diff --git a/codi-rs/src/tui/app.rs b/codi-rs/src/tui/app.rs index c5705ad..30b5908 100644 --- a/codi-rs/src/tui/app.rs +++ b/codi-rs/src/tui/app.rs @@ -152,6 +152,8 @@ pub enum AppEvent { TurnComplete(TurnStats), /// Confirmation request. ConfirmRequest(ToolConfirmation), + /// Context compaction started (true) or finished (false). + Compaction(bool), } /// Pending tool confirmation. @@ -270,6 +272,10 @@ impl App { } /// Create with a provider. + /// + /// **Deprecated**: Bypasses config wiring. Use `with_project_path()` + + /// `set_config()` + `set_provider()` instead (see `run_repl()`). + #[deprecated(note = "bypasses config; use with_project_path() + set_config() + set_provider()")] pub fn with_provider(provider: BoxedProvider) -> Self { let mut app = Self::new(); app.set_provider(provider); @@ -277,6 +283,10 @@ impl App { } /// Create with a provider and project path. + /// + /// **Deprecated**: Bypasses config wiring. Use `with_project_path()` + + /// `set_config()` + `set_provider()` instead (see `run_repl()`). + #[deprecated(note = "bypasses config; use with_project_path() + set_config() + set_provider()")] pub fn with_provider_and_path(provider: BoxedProvider, project_path: impl AsRef) -> Self { let mut app = Self::with_project_path(project_path); app.set_provider(provider); @@ -305,6 +315,7 @@ impl App { extract_tools_from_text: config.extract_tools_from_text, auto_approve_all: self.auto_approve_all, auto_approve_tools: config.auto_approve.clone(), + dangerous_patterns: config.dangerous_patterns.clone(), } } else { let mut default_config = AgentConfig::default(); @@ -315,21 +326,7 @@ impl App { /// Build the system prompt, incorporating config additions and project context. fn build_system_prompt(&self) -> String { - let mut prompt = "You are Codi, a helpful AI coding assistant. Help the user with their programming tasks.".to_string(); - - if let Some(ref config) = self.config { - if let Some(ref additions) = config.system_prompt_additions { - prompt.push_str("\n\n"); - prompt.push_str(additions); - } - - if let Some(ref project_context) = config.project_context { - prompt.push_str("\n\n## Project Context\n"); - prompt.push_str(project_context); - } - } - - prompt + build_system_prompt_from_config(self.config.as_ref()) } /// Set the AI provider and create an agent. @@ -357,7 +354,12 @@ impl App { } })), on_confirm: None, // Handled via channel-based approach - on_compaction: None, + on_compaction: Some(Arc::new({ + let tx = event_tx.clone(); + move |is_starting: bool| { + let _ = tx.send(AppEvent::Compaction(is_starting)); + } + })), on_turn_complete: Some(Arc::new({ let tx = event_tx.clone(); move |stats: &TurnStats| { @@ -478,6 +480,13 @@ impl App { AppEvent::ConfirmRequest(_) => { // Handled separately via channel } + AppEvent::Compaction(is_starting) => { + if is_starting { + self.status = Some("Compacting context...".to_string()); + } else { + self.status = Some("Context compacted".to_string()); + } + } } } } @@ -1290,6 +1299,29 @@ impl App { } } +/// Build a system prompt from an optional `ResolvedConfig`. +/// +/// This is the standalone version used by both the TUI (`App::build_system_prompt`) +/// and the non-interactive `-P` mode so that config-driven prompt additions are +/// applied consistently. +pub fn build_system_prompt_from_config(config: Option<&ResolvedConfig>) -> String { + let mut prompt = "You are Codi, a helpful AI coding assistant. Help the user with their programming tasks.".to_string(); + + if let Some(config) = config { + if let Some(ref additions) = config.system_prompt_additions { + prompt.push_str("\n\n"); + prompt.push_str(additions); + } + + if let Some(ref project_context) = config.project_context { + prompt.push_str("\n\n## Project Context\n"); + prompt.push_str(project_context); + } + } + + prompt +} + /// Format worker status for display. fn format_worker_status(status: &WorkerStatus) -> String { match status { @@ -1488,4 +1520,39 @@ mod tests { assert_eq!(app.input, "current"); assert_eq!(app.history_index, None); } + + #[test] + fn test_build_system_prompt_from_config_none() { + let prompt = build_system_prompt_from_config(None); + assert!(prompt.contains("Codi")); + assert!(!prompt.contains("Project Context")); + } + + #[test] + fn test_build_system_prompt_from_config_with_additions() { + let mut config = crate::config::default_config(); + config.system_prompt_additions = Some("Be concise.".to_string()); + config.project_context = Some("Rust CLI app.".to_string()); + + let prompt = build_system_prompt_from_config(Some(&config)); + assert!(prompt.contains("Be concise.")); + assert!(prompt.contains("## Project Context")); + assert!(prompt.contains("Rust CLI app.")); + } + + #[test] + fn test_app_event_compaction_variant() { + // Verify the Compaction variant exists and can be constructed + let start = AppEvent::Compaction(true); + let end = AppEvent::Compaction(false); + // Pattern match to confirm the variant works + match start { + AppEvent::Compaction(is_starting) => assert!(is_starting), + _ => panic!("expected Compaction variant"), + } + match end { + AppEvent::Compaction(is_starting) => assert!(!is_starting), + _ => panic!("expected Compaction variant"), + } + } } diff --git a/codi-rs/src/tui/mod.rs b/codi-rs/src/tui/mod.rs index 8e270da..44d55b4 100644 --- a/codi-rs/src/tui/mod.rs +++ b/codi-rs/src/tui/mod.rs @@ -41,7 +41,7 @@ pub mod events; pub mod streaming; pub mod ui; -pub use app::{App, AppMode, Message as ChatMessage}; +pub use app::{App, AppMode, Message as ChatMessage, build_system_prompt_from_config}; pub use events::{Event, EventHandler}; pub use streaming::{MarkdownStreamCollector, StreamController, StreamState, StreamStatus};