diff --git a/FEATURE_PARITY.md b/FEATURE_PARITY.md index 83d6d65e..6a766bd2 100644 --- a/FEATURE_PARITY.md +++ b/FEATURE_PARITY.md @@ -39,7 +39,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O | Network modes (loopback/LAN/remote) | ✅ | 🚧 | HTTP only | | OpenAI-compatible HTTP API | ✅ | ✅ | /v1/chat/completions, per-request `model` override | | Canvas hosting | ✅ | ❌ | Agent-driven UI | -| Gateway lock (PID-based) | ✅ | ❌ | | +| Gateway lock (PID-based) | ✅ | ✅ | `fs4` flock-based, acquired in `main.rs` before agent startup | | launchd/systemd integration | ✅ | ❌ | | | Bonjour/mDNS discovery | ✅ | ❌ | | | Tailscale integration | ✅ | ❌ | | diff --git a/docker-compose.yml b/docker-compose.yml index 9b82fcdb..e3e6f578 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -3,7 +3,7 @@ services: postgres: image: pgvector/pgvector:pg16 ports: - - "5432:5432" + - "127.0.0.1:5432:5432" environment: POSTGRES_DB: ironclaw POSTGRES_USER: ironclaw diff --git a/src/app.rs b/src/app.rs index a2acd9c0..43630c32 100644 --- a/src/app.rs +++ b/src/app.rs @@ -255,15 +255,18 @@ impl AppBuilder { self.libsql_db.take(); } - // Re-resolve config with OS credentials - if let Some(ref db) = self.db { - let toml_path = self.toml_path.as_deref(); - if let Ok(refreshed) = - Config::from_db_with_toml(db.as_ref(), "default", toml_path).await - { - self.config = refreshed; - tracing::debug!("LlmConfig re-resolved after OS credential injection"); - } + // Re-resolve only the LLM config with OS credentials. + let store: Option<&(dyn crate::db::SettingsStore + Sync)> = + self.db.as_ref().map(|db| db.as_ref() as _); + let toml_path = self.toml_path.as_deref(); + if let Err(e) = self + .config + .re_resolve_llm(store, "default", toml_path) + .await + { + tracing::warn!( + "Failed to re-resolve LLM config after OS credential injection: {e}" + ); } return Ok(()); @@ -308,18 +311,16 @@ impl AppBuilder { // Inject LLM API keys from encrypted storage crate::config::inject_llm_keys_from_secrets(secrets.as_ref(), "default").await; - // Re-resolve config with newly available keys - if let Some(ref db) = self.db { - let toml_path = self.toml_path.as_deref(); - match Config::from_db_with_toml(db.as_ref(), "default", toml_path).await { - Ok(refreshed) => { - self.config = refreshed; - tracing::debug!("LlmConfig re-resolved after secret injection"); - } - Err(e) => { - tracing::warn!("Failed to re-resolve config after secret injection: {}", e); - } - } + // Re-resolve only the LLM config with newly available keys. + let store: Option<&(dyn crate::db::SettingsStore + Sync)> = + self.db.as_ref().map(|db| db.as_ref() as _); + let toml_path = self.toml_path.as_deref(); + if let Err(e) = self + .config + .re_resolve_llm(store, "default", toml_path) + .await + { + tracing::warn!("Failed to re-resolve LLM config after secret injection: {e}"); } } diff --git a/src/bootstrap.rs b/src/bootstrap.rs index f9ca6fd5..899b96cc 100644 --- a/src/bootstrap.rs +++ b/src/bootstrap.rs @@ -414,10 +414,103 @@ pub enum MigrationError { Io(String), } +// ── PID Lock ────────────────────────────────────────────────────────────── + +/// Path to the PID lock file: `~/.ironclaw/ironclaw.pid`. +pub fn pid_lock_path() -> PathBuf { + ironclaw_base_dir().join("ironclaw.pid") +} + +/// A PID-based lock that prevents multiple IronClaw instances from running +/// simultaneously. +/// +/// Uses `fs4::try_lock_exclusive()` for atomic locking (no TOCTOU race), +/// then writes the current PID into the locked file for diagnostics. +/// The OS-level lock is held for the lifetime of this struct and +/// automatically released on drop (along with the PID file cleanup). +#[derive(Debug)] +pub struct PidLock { + path: PathBuf, + /// Held open to maintain the OS-level exclusive lock. + _file: std::fs::File, +} + +/// Errors from PID lock acquisition. +#[derive(Debug, thiserror::Error)] +pub enum PidLockError { + #[error("Another IronClaw instance is already running (PID {pid})")] + AlreadyRunning { pid: u32 }, + #[error("Failed to acquire PID lock: {0}")] + Io(#[from] std::io::Error), +} + +impl PidLock { + /// Try to acquire the PID lock. + /// + /// Uses an exclusive file lock (`flock`/`LockFileEx`) so that two + /// concurrent processes cannot both acquire the lock — no TOCTOU race. + /// If the lock file exists but the holding process is gone (stale), + /// the lock is reclaimed automatically by the OS. + pub fn acquire() -> Result { + Self::acquire_at(pid_lock_path()) + } + + /// Acquire at a specific path (for testing). + fn acquire_at(path: PathBuf) -> Result { + use fs4::FileExt; + use std::fs::OpenOptions; + use std::io::Write; + + // Ensure parent directory exists + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + + // Open (or create) the lock file + let mut file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(&path)?; + + // Try non-blocking exclusive lock — if another process holds it, + // this fails immediately instead of blocking. + if let Err(e) = file.try_lock_exclusive() { + if e.kind() == std::io::ErrorKind::WouldBlock { + // Lock held by another process — read its PID for the error message + let pid = std::fs::read_to_string(&path) + .ok() + .and_then(|s| s.trim().parse::().ok()) + .unwrap_or(0); + return Err(PidLockError::AlreadyRunning { pid }); + } + // Other errors (permissions, unsupported filesystem, etc.) + return Err(PidLockError::Io(e)); + } + + // We hold the exclusive lock — write our PID + file.set_len(0)?; // truncate + write!(file, "{}", std::process::id())?; + + Ok(PidLock { path, _file: file }) + } +} + +impl Drop for PidLock { + fn drop(&mut self) { + // Remove the PID file; the OS-level lock is released when _file is dropped. + let _ = std::fs::remove_file(&self.path); + } +} + #[cfg(test)] mod tests { use super::*; + use std::process::Command; use std::sync::Mutex; + use std::thread; + use std::time::{Duration, Instant}; use tempfile::tempdir; static ENV_MUTEX: Mutex<()> = Mutex::new(()); @@ -986,4 +1079,162 @@ INJECTED="pwned"#; unsafe { std::env::remove_var("IRONCLAW_BASE_DIR") }; } } + + // ── PID Lock tests ─────────────────────────────────────────────── + + #[test] + fn test_pid_lock_acquire_and_drop() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("ironclaw.pid"); + + // Acquire lock + let lock = PidLock::acquire_at(pid_path.clone()).unwrap(); + assert!(pid_path.exists()); + + // PID file should contain our PID + let contents = std::fs::read_to_string(&pid_path).unwrap(); + assert_eq!(contents.trim().parse::().unwrap(), std::process::id()); + + // Drop should remove the file + drop(lock); + assert!(!pid_path.exists()); + } + + #[test] + fn test_pid_lock_rejects_second_acquire() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("ironclaw.pid"); + + // First lock succeeds + let _lock1 = PidLock::acquire_at(pid_path.clone()).unwrap(); + + // Second acquire on same file must fail (exclusive flock held) + let result = PidLock::acquire_at(pid_path.clone()); + assert!(result.is_err()); + match result.unwrap_err() { + PidLockError::AlreadyRunning { pid } => { + assert_eq!(pid, std::process::id()); + } + other => panic!("expected AlreadyRunning, got: {}", other), + } + } + + #[test] + fn test_pid_lock_reclaims_after_drop() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("ironclaw.pid"); + + // Acquire and release + let lock = PidLock::acquire_at(pid_path.clone()).unwrap(); + drop(lock); + + // Should succeed — OS lock was released on drop + let lock2 = PidLock::acquire_at(pid_path).unwrap(); + drop(lock2); + } + + #[test] + fn test_pid_lock_reclaims_stale_file_without_flock() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("ironclaw.pid"); + + // Write a stale PID file manually (no flock held) + std::fs::write(&pid_path, "4294967294").unwrap(); + + // Should succeed because no OS lock is held on the file + let lock = PidLock::acquire_at(pid_path.clone()).unwrap(); + let contents = std::fs::read_to_string(&pid_path).unwrap(); + assert_eq!(contents.trim().parse::().unwrap(), std::process::id()); + drop(lock); + } + + #[test] + fn test_pid_lock_handles_corrupt_pid_file() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("ironclaw.pid"); + + // Write garbage (no flock held) + std::fs::write(&pid_path, "not-a-number").unwrap(); + + // Should succeed — no OS lock held, file is reclaimed + let lock = PidLock::acquire_at(pid_path).unwrap(); + drop(lock); + } + + #[test] + fn test_pid_lock_creates_parent_dirs() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("nested").join("deep").join("ironclaw.pid"); + + let lock = PidLock::acquire_at(pid_path.clone()).unwrap(); + assert!(pid_path.exists()); + drop(lock); + } + + #[test] + fn test_pid_lock_child_helper_holds_lock() { + if std::env::var("IRONCLAW_PID_LOCK_CHILD").ok().as_deref() != Some("1") { + return; + } + + let pid_path = PathBuf::from( + std::env::var("IRONCLAW_PID_LOCK_PATH").expect("IRONCLAW_PID_LOCK_PATH missing"), + ); + let hold_ms = std::env::var("IRONCLAW_PID_LOCK_HOLD_MS") + .ok() + .and_then(|s| s.parse::().ok()) + .unwrap_or(3000); + + let _lock = PidLock::acquire_at(pid_path).expect("child failed to acquire pid lock"); + thread::sleep(Duration::from_millis(hold_ms)); + } + + #[test] + fn test_pid_lock_rejects_lock_held_by_other_process() { + let dir = tempdir().unwrap(); + let pid_path = dir.path().join("ironclaw.pid"); + + let current_exe = std::env::current_exe().unwrap(); + let mut child = Command::new(current_exe) + .args([ + "--exact", + "bootstrap::tests::test_pid_lock_child_helper_holds_lock", + "--nocapture", + "--test-threads=1", + ]) + .env("IRONCLAW_PID_LOCK_CHILD", "1") + .env("IRONCLAW_PID_LOCK_PATH", pid_path.display().to_string()) + .env("IRONCLAW_PID_LOCK_HOLD_MS", "3000") + .spawn() + .unwrap(); + + let started = Instant::now(); + while started.elapsed() < Duration::from_secs(2) { + if pid_path.exists() { + break; + } + if let Some(status) = child.try_wait().unwrap() { + panic!("child exited before acquiring lock: {}", status); + } + thread::sleep(Duration::from_millis(20)); + } + assert!( + pid_path.exists(), + "child did not create lock file in time: {}", + pid_path.display() + ); + + let result = PidLock::acquire_at(pid_path.clone()); + match result.unwrap_err() { + PidLockError::AlreadyRunning { .. } => {} + other => panic!("expected AlreadyRunning, got: {}", other), + } + + let status = child.wait().unwrap(); + assert!(status.success(), "child process failed: {}", status); + + // After the child exits, lock should be released and reacquirable. + let lock = PidLock::acquire_at(pid_path).unwrap(); + drop(lock); + } } diff --git a/src/channels/repl.rs b/src/channels/repl.rs index 9031aa4e..b1f06ec2 100644 --- a/src/channels/repl.rs +++ b/src/channels/repl.rs @@ -18,7 +18,7 @@ //! - `Esc` - Interrupt current operation use std::borrow::Cow; -use std::io::{self, Write}; +use std::io::{self, IsTerminal, Write}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; @@ -303,6 +303,9 @@ impl Channel for ReplChannel { if let Some(msg) = single_message { let incoming = IncomingMessage::new("repl", "default", &msg).with_timezone(&sys_tz); let _ = tx.blocking_send(incoming); + // Ensure the agent exits after handling exactly one turn in -m mode, + // even when other channels (gateway/http) are enabled. + let _ = tx.blocking_send(IncomingMessage::new("repl", "default", "/quit")); return; } @@ -408,10 +411,15 @@ impl Channel for ReplChannel { } } Err(ReadlineError::Eof) => { - // Ctrl+D: send /quit so the agent loop runs graceful shutdown - let msg = - IncomingMessage::new("repl", "default", "/quit").with_timezone(&sys_tz); - let _ = tx.blocking_send(msg); + // Ctrl+D in interactive mode: graceful shutdown. + // In daemon mode (stdin = /dev/null, no TTY), EOF arrives + // immediately — just drop the REPL thread silently so other + // channels (gateway, telegram, …) keep running. + if std::io::stdin().is_terminal() { + let msg = IncomingMessage::new("repl", "default", "/quit") + .with_timezone(&sys_tz); + let _ = tx.blocking_send(msg); + } break; } Err(e) => { @@ -621,3 +629,29 @@ impl Channel for ReplChannel { Ok(()) } } + +#[cfg(test)] +mod tests { + use futures::StreamExt; + + use super::*; + + #[tokio::test] + async fn single_message_mode_sends_message_then_quit() { + let repl = ReplChannel::with_message("hi".to_string()); + let mut stream = repl.start().await.expect("repl start should succeed"); + + let first = stream.next().await.expect("first message missing"); + assert_eq!(first.channel, "repl"); + assert_eq!(first.content, "hi"); + + let second = stream.next().await.expect("quit message missing"); + assert_eq!(second.channel, "repl"); + assert_eq!(second.content, "/quit"); + + assert!( + stream.next().await.is_none(), + "stream should end after /quit" + ); + } +} diff --git a/src/channels/web/static/app.js b/src/channels/web/static/app.js index 68b803f9..87c83ede 100644 --- a/src/channels/web/static/app.js +++ b/src/channels/web/static/app.js @@ -1481,7 +1481,7 @@ chatInput.addEventListener('keydown', (e) => { } } - if (e.key === 'Enter' && !e.shiftKey) { + if (e.key === 'Enter' && !e.shiftKey && !e.isComposing) { e.preventDefault(); hideSlashAutocomplete(); sendMessage(); diff --git a/src/cli/status.rs b/src/cli/status.rs index 4a495141..de17a226 100644 --- a/src/cli/status.rs +++ b/src/cli/status.rs @@ -8,9 +8,35 @@ use std::path::PathBuf; use crate::bootstrap::ironclaw_base_dir; use crate::settings::Settings; +/// Load settings from JSON and TOML config files, matching the runtime +/// priority: TOML overlay > settings.json > defaults. +/// +/// This mirrors the loading chain in `Config::from_env_with_toml()` but +/// without resolving the full `Config` (which requires async + secrets). +fn load_settings() -> Settings { + load_settings_from(&Settings::default_path(), &Settings::default_toml_path()) +} + +/// Inner implementation with injectable paths (testable). +fn load_settings_from(json_path: &std::path::Path, toml_path: &std::path::Path) -> Settings { + let mut settings = Settings::load_from(json_path); + + match Settings::load_toml(toml_path) { + Ok(Some(toml_settings)) => { + settings.merge_from(&toml_settings); + } + Ok(None) => {} // File not found — fine for default path + Err(e) => { + eprintln!("Warning: failed to parse {}: {}", toml_path.display(), e); + } + } + + settings +} + /// Run the status command, printing system health info. pub async fn run_status_command() -> anyhow::Result<()> { - let settings = Settings::default(); + let settings = load_settings(); println!("IronClaw Status"); println!("===============\n"); @@ -209,3 +235,99 @@ fn default_tools_dir() -> PathBuf { fn default_channels_dir() -> PathBuf { ironclaw_base_dir().join("channels") } + +#[cfg(test)] +mod tests { + use super::load_settings_from; + + /// Regression test for #354: load_settings_from must read config.toml. + #[test] + fn reads_toml_heartbeat_enabled() { + let dir = tempfile::tempdir().expect("tempdir"); + let json_path = dir.path().join("settings.json"); + let toml_path = dir.path().join("config.toml"); + + // No JSON file — only TOML + std::fs::write( + &toml_path, + "[heartbeat]\nenabled = true\ninterval_secs = 600", + ) + .expect("write toml"); + + let settings = load_settings_from(&json_path, &toml_path); + assert!(settings.heartbeat.enabled); + assert_eq!(settings.heartbeat.interval_secs, 600); + } + + /// Without any config files, defaults are returned. + #[test] + fn defaults_without_config_files() { + let dir = tempfile::tempdir().expect("tempdir"); + let settings = load_settings_from( + &dir.path().join("nonexistent.json"), + &dir.path().join("nonexistent.toml"), + ); + assert!(!settings.heartbeat.enabled); + } + + /// settings.json is respected. + #[test] + fn reads_json_heartbeat_enabled() { + let dir = tempfile::tempdir().expect("tempdir"); + let json_path = dir.path().join("settings.json"); + let toml_path = dir.path().join("nonexistent.toml"); + + std::fs::write( + &json_path, + r#"{"heartbeat":{"enabled":true,"interval_secs":900}}"#, + ) + .expect("write json"); + + let settings = load_settings_from(&json_path, &toml_path); + assert!(settings.heartbeat.enabled); + assert_eq!(settings.heartbeat.interval_secs, 900); + } + + /// TOML overlay wins over JSON settings. + #[test] + fn toml_overlay_wins_over_json() { + let dir = tempfile::tempdir().expect("tempdir"); + let json_path = dir.path().join("settings.json"); + let toml_path = dir.path().join("config.toml"); + + std::fs::write( + &json_path, + r#"{"heartbeat":{"enabled":false,"interval_secs":100}}"#, + ) + .expect("write json"); + std::fs::write( + &toml_path, + "[heartbeat]\nenabled = true\ninterval_secs = 200", + ) + .expect("write toml"); + + let settings = load_settings_from(&json_path, &toml_path); + assert!(settings.heartbeat.enabled); + assert_eq!(settings.heartbeat.interval_secs, 200); + } + + /// Invalid TOML is warned but doesn't crash; falls back to JSON/defaults. + #[test] + fn invalid_toml_falls_back_gracefully() { + let dir = tempfile::tempdir().expect("tempdir"); + let json_path = dir.path().join("settings.json"); + let toml_path = dir.path().join("config.toml"); + + std::fs::write( + &json_path, + r#"{"heartbeat":{"enabled":true,"interval_secs":500}}"#, + ) + .expect("write json"); + std::fs::write(&toml_path, "this is not valid toml [[[").expect("write bad toml"); + + let settings = load_settings_from(&json_path, &toml_path); + // Should fall back to JSON values, not crash + assert!(settings.heartbeat.enabled); + assert_eq!(settings.heartbeat.interval_secs, 500); + } +} diff --git a/src/config/mod.rs b/src/config/mod.rs index 736ca9a8..06852be7 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -262,6 +262,32 @@ impl Config { Ok(()) } + /// Re-resolve only the LLM config after credential injection. + /// + /// Called by `AppBuilder::init_secrets()` after injecting API keys into + /// the env overlay. Only rebuilds `self.llm` — all other config fields + /// are unaffected, preserving values from the initial config load (or + /// from `Config::for_testing()` in test mode). + pub async fn re_resolve_llm( + &mut self, + store: Option<&(dyn crate::db::SettingsStore + Sync)>, + user_id: &str, + toml_path: Option<&std::path::Path>, + ) -> Result<(), ConfigError> { + let settings = if let Some(store) = store { + let mut s = match store.get_all_settings(user_id).await { + Ok(map) => Settings::from_db_map(&map), + Err(_) => Settings::default(), + }; + Self::apply_toml_overlay(&mut s, toml_path)?; + s + } else { + Settings::default() + }; + self.llm = LlmConfig::resolve(&settings)?; + Ok(()) + } + /// Build config from settings (shared by from_env and from_db). async fn build(settings: &Settings) -> Result { Ok(Self { diff --git a/src/main.rs b/src/main.rs index 814d26a9..9e79d378 100644 --- a/src/main.rs +++ b/src/main.rs @@ -145,6 +145,24 @@ async fn async_main() -> anyhow::Result<()> { } } + // ── PID lock (prevent multiple instances) ──────────────────────── + let _pid_lock = match ironclaw::bootstrap::PidLock::acquire() { + Ok(lock) => Some(lock), + Err(ironclaw::bootstrap::PidLockError::AlreadyRunning { pid }) => { + anyhow::bail!( + "Another IronClaw instance is already running (PID {}). \ + If this is incorrect, remove the stale PID file: {}", + pid, + ironclaw::bootstrap::pid_lock_path().display() + ); + } + Err(e) => { + eprintln!("Warning: Could not acquire PID lock: {}", e); + eprintln!("Continuing without PID lock protection."); + None + } + }; + // ── Agent startup ────────────────────────────────────────────────── // Enhanced first-run detection @@ -166,13 +184,12 @@ async fn async_main() -> anyhow::Result<()> { let config = match Config::from_env_with_toml(toml_path).await { Ok(c) => c, Err(ironclaw::error::ConfigError::MissingRequired { key, hint }) => { - eprintln!("Configuration error: Missing required setting '{}'", key); - eprintln!(" {}", hint); - eprintln!(); - eprintln!( - "Run 'ironclaw onboard' to configure, or set the required environment variables." + anyhow::bail!( + "Configuration error: Missing required setting '{}'. {}. \ + Run 'ironclaw onboard' to configure, or set the required environment variables.", + key, + hint ); - std::process::exit(1); } Err(e) => return Err(e.into()), }; diff --git a/tests/e2e_builtin_tool_coverage.rs b/tests/e2e_builtin_tool_coverage.rs index 2143d7a9..c5ce339b 100644 --- a/tests/e2e_builtin_tool_coverage.rs +++ b/tests/e2e_builtin_tool_coverage.rs @@ -242,6 +242,16 @@ mod tests { "create_job should return a job_id: {:?}", create_result.1 ); + assert!( + create_result.1.contains("in_progress"), + "create_job should dispatch through the scheduler, not stay pending: {:?}", + create_result.1 + ); + assert!( + !create_result.1.contains("scheduler unavailable"), + "create_job should not fall back to the unscheduled path: {:?}", + create_result.1 + ); let status_result = results .iter() .find(|(n, _)| n == "job_status") diff --git a/tests/support/test_rig.rs b/tests/support/test_rig.rs index 0073741e..f21b5d7c 100644 --- a/tests/support/test_rig.rs +++ b/tests/support/test_rig.rs @@ -545,16 +545,14 @@ impl TestRigBuilder { .await .expect("AppBuilder::build_all() failed in test rig"); + let scheduler_slot: ironclaw::tools::builtin::SchedulerSlot = + Arc::new(tokio::sync::RwLock::new(None)); + // 6. Register job tools, routine tools, and extra tools. { - use ironclaw::context::ContextManager; - - let ctx_mgr = Arc::new(ContextManager::new( - components.config.agent.max_parallel_jobs, - )); components.tools.register_job_tools( - ctx_mgr, - None, + Arc::clone(&components.context_manager), + Some(scheduler_slot.clone()), None, components.db.clone(), None, @@ -657,10 +655,13 @@ impl TestRigBuilder { None, // heartbeat_config None, // hygiene_config routine_config, - None, // context_manager + Some(Arc::clone(&components.context_manager)), None, // session_manager ); + // Match main.rs: fill the scheduler slot once Agent::new has created it. + *scheduler_slot.write().await = Some(agent.scheduler()); + // 9. Spawn agent in background task. let agent_handle = tokio::spawn(async move { if let Err(e) = agent.run().await { diff --git a/tests/support/trace_llm.rs b/tests/support/trace_llm.rs index 804e8eab..e09ee9d9 100644 --- a/tests/support/trace_llm.rs +++ b/tests/support/trace_llm.rs @@ -513,32 +513,41 @@ impl LlmProvider for TraceLlm { } async fn complete(&self, request: CompletionRequest) -> Result { - let step = self.next_step(&request.messages)?; - match step.response { - TraceResponse::Text { - content, - input_tokens, - output_tokens, - } => Ok(CompletionResponse { - content, - input_tokens, - output_tokens, - finish_reason: FinishReason::Stop, - cache_read_input_tokens: 0, - cache_creation_input_tokens: 0, - }), - TraceResponse::ToolCalls { .. } => Err(LlmError::RequestFailed { - provider: self.model_name.clone(), - reason: "TraceLlm::complete() called but current step is a tool_calls response; \ - use complete_with_tools() instead" - .to_string(), - }), - TraceResponse::UserInput { .. } => Err(LlmError::RequestFailed { - provider: self.model_name.clone(), - reason: "TraceLlm::complete() encountered a user_input step; \ - these should have been filtered out during construction" - .to_string(), - }), + // complete() is called when Reasoning has force_text=true (no tools + // available). Skip any remaining ToolCalls steps in the trace and + // return the next Text step, since in real usage the LLM would + // produce text when no tools are offered. + loop { + let step = self.next_step(&request.messages)?; + match step.response { + TraceResponse::Text { + content, + input_tokens, + output_tokens, + } => { + return Ok(CompletionResponse { + content, + input_tokens, + output_tokens, + finish_reason: FinishReason::Stop, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + }); + } + TraceResponse::ToolCalls { .. } => { + // Skip tool_calls steps — complete() is called in + // force_text mode so the LLM can't use tools anyway. + continue; + } + TraceResponse::UserInput { .. } => { + return Err(LlmError::RequestFailed { + provider: self.model_name.clone(), + reason: "TraceLlm::complete() encountered a user_input step; \ + these should have been filtered out during construction" + .to_string(), + }); + } + } } } diff --git a/tests/support_unit_tests.rs b/tests/support_unit_tests.rs index 645746ea..4ac65c0f 100644 --- a/tests/support_unit_tests.rs +++ b/tests/support_unit_tests.rs @@ -571,22 +571,29 @@ mod trace_llm_tests { } #[tokio::test] - async fn complete_errors_on_tool_calls_step() { + async fn complete_skips_tool_calls_step() { + // complete() is called in force_text mode where tools aren't available. + // When the trace has a ToolCalls step followed by a Text step, complete() + // should skip the ToolCalls and return the Text response. let trace = LlmTrace::single_turn( "test-model", "hi", - vec![tool_calls_step(vec![simple_tool_call("echo")], 10, 5)], + vec![ + tool_calls_step(vec![simple_tool_call("echo")], 10, 5), + text_step("skipped past tools", 20, 8), + ], ); let llm = TraceLlm::from_trace(trace); - let result = llm.complete(make_completion_request("hi")).await; + let resp = llm + .complete(make_completion_request("hi")) + .await + .expect("complete() should skip ToolCalls and return the Text step"); - assert!(result.is_err()); - let err_msg = result.unwrap_err().to_string(); - assert!( - err_msg.contains("tool_calls"), - "Expected 'tool_calls' in error: {err_msg}" - ); + assert_eq!(resp.content, "skipped past tools"); + assert_eq!(resp.input_tokens, 20); + assert_eq!(resp.output_tokens, 8); + assert_eq!(resp.finish_reason, FinishReason::Stop); } #[tokio::test]