From 86ae12747bd872ea9ba8a210324b0004f4d96662 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Thu, 19 Mar 2026 13:37:55 -0700 Subject: [PATCH 1/3] feat: LRU embedding cache for workspace search (#1423) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: LRU embedding cache for workspace search (#165) Add CachedEmbeddingProvider that wraps any EmbeddingProvider with an in-memory LRU cache keyed by SHA-256(model_name + text). This avoids redundant HTTP calls when the same text is embedded multiple times (common during reindexing and repeated searches). - Cache uses HashMap + last_accessed tracking with manual LRU eviction (same pattern as llm::response_cache::CachedProvider) - Lock is never held during HTTP calls to prevent blocking - embed_batch() partitions into hits/misses and only fetches misses - Default 10,000 entries (~58 MB for 1536-dim vectors) - Configurable via EMBEDDING_CACHE_SIZE env var - Workspace.with_embeddings() auto-wraps; with_embeddings_uncached() available for tests Co-Authored-By: Claude Opus 4.6 * fix: address review comments on embedding cache - Validate embed_batch return count matches expected miss count - Replace unwrap_or_default() with proper error propagation - Fix batch eviction: run final eviction pass after insert to enforce cap - Fix test: use different-length inputs to verify ordering correctness - Reject EMBEDDING_CACHE_SIZE=0 in config validation (minimum is 1) Co-Authored-By: Claude Opus 4.6 * fix: replace .expect() with proper error handling in embed_batch The all-cache-hits early-return path used .expect("all cache hits") which violates the project convention of no .unwrap()/.expect() in production code. Replaced with the same ok_or_else pattern used in the normal path. Co-Authored-By: Claude Opus 4.6 * fix: clarify memory sizing docs and use saturating_add for eviction - Update memory comments in embedding_cache.rs, config/embeddings.rs, and workspace/mod.rs to note the ~58 MB figure is payload-only (actual memory is higher due to HashMap/key/allocation overhead) - Use saturating_add(1) instead of + 1 for eviction threshold to prevent overflow if max_entries is usize::MAX Co-Authored-By: Claude Opus 4.6 * fix: address Copilot review on embedding cache - Avoid double-clone per miss in embed_batch: move embedding into results, clone only for the cache entry - Evict per-insert instead of after all inserts to keep peak memory bounded during large batches - Clamp max_entries to at least 1 in constructor to prevent unexpected eviction behavior when set to 0 via the public API Co-Authored-By: Claude Opus 4.6 * fix: reduce embedding_cache module visibility to private Types are already re-exported via `pub use`, so the module itself doesn't need to be public. Reduces unnecessary API surface. Co-Authored-By: Claude Opus 4.6 * fix: address serrrfirat review feedback on embedding cache - Add TODO comment for O(n) LRU eviction scalability - Add thundering herd note at lock release in embed() - Warn when cache max_entries exceeds 100k - Use with_embeddings_uncached() in integration test - Add tests: error_does_not_pollute_cache, embed_batch_empty_input - Update README with cache-aware with_embeddings() docs Co-Authored-By: Claude Opus 4.6 * fix: prevent u32 wrapping in FailThenSucceedMock failure counter fetch_sub(1) wraps to u32::MAX when called past zero, silently breaking the mock for 3+ calls. Switch to load-then-store to avoid the wrapping bug in both embed() and embed_batch(). Co-Authored-By: Claude Opus 4.6 * fix: address Copilot and serrrfirat review findings on embedding cache - Switch tokio::sync::Mutex to std::sync::Mutex (lock never held across .await — cheaper synchronous lock) - Extract DEFAULT_EMBEDDING_CACHE_SIZE constant to avoid 10_000 duplication between EmbeddingCacheConfig and EmbeddingsConfig Co-Authored-By: Claude Opus 4.6 * test: add all-misses batch test for embedding cache Adds embed_batch_all_misses test covering the case where every text in a batch is a cache miss — fulfilling the commitment from serrrfirat's review. Co-Authored-By: Claude Opus 4.6 * chore: trigger CI re-check after rebase * fix: use raw [u8;32] cache keys and pre-allocate HashMap capacity Address Copilot review findings: - cache_key() now returns [u8; 32] instead of hex String, avoiding a 64-byte allocation per lookup - HashMap::with_capacity(max_entries) avoids incremental reallocation - Fix pre-existing staging compilation error in cli/routines.rs (missing max_tool_rounds/use_tools fields) [skip-regression-check] * fix: make cache accessors sync and update doc for [u8;32] keys Address Copilot review: - len(), is_empty(), clear() are now sync since they only take a std::sync::Mutex lock with no .await points - Update cache_size doc comment to reflect [u8;32] keys instead of String keys [skip-regression-check] * fix: remove clone_on_copy for [u8; 32] cache keys [skip-regression-check] * ci: add safety comments to test code for no-panics check The CI no-panics grep check cannot distinguish test code inside src/ files from production code. Add // safety: test annotations to .unwrap(), .expect(), and assert!() calls in #[cfg(test)] modules. * fix: correct cache doc and demote hit/miss logs to trace - Fix misleading "String keys" in memory comment (cache uses [u8; 32]) - Demote per-request hit/miss logs from debug to trace to reduce noise on hot paths (batch summary stays at trace too) * docs: add missing Arc import in workspace README example * perf: batch eviction in embed_batch to avoid O(n×m) cost Replace per-insert evict_lru call with a single evict_k_oldest pass that computes eviction count upfront and removes the k oldest entries in one O(n) scan. Avoids O(n×m) HashMap iterations while holding the mutex during batch inserts. * fix: cap batch cache inserts at max_entries and use O(n) selection - evict_k_oldest now uses select_nth_unstable_by_key for O(n) average partial selection instead of O(n log n) full sort - embed_batch caps cached entries at max_entries when misses exceed capacity, preventing the cache from growing unbounded - Added test: batch_exceeding_capacity_respects_max_entries * fix: flatten test assert for fmt compatibility Shorten assert message to fit single line so cargo fmt doesn't split the safety annotation onto a separate line. * fix: address review feedback and improve embedding cache (takeover #235) - Fix merge conflict: add missing allow_always field in PendingApproval - Thread EmbeddingCacheConfig through CLI memory commands so they respect EMBEDDING_CACHE_SIZE instead of silently using default (fixes #235 review) - Cap HashMap pre-allocation at min(max_entries, 1024) to avoid upfront memory waste at large cache sizes - Fix FailThenSucceedMock race: replace load+store with atomic fetch_update - Remove noisy '// safety: test' comments (40+ lines of diff noise) - Fix collapsed lines from comment removal - Simplify redundant Ok(...collect()?) to just collect() Co-Authored-By: ztsalexey Co-Authored-By: Claude Opus 4.6 (1M context) * fix(embedding-cache): skip eviction on concurrent duplicate insert When the lock is released for the HTTP call, another caller may insert the same key. Re-check under lock and just update the existing entry without evicting, avoiding unnecessary cache churn under concurrency. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: ztsalexey Co-authored-by: Claude Opus 4.6 Co-authored-by: ztsalexey --- src/agent/thread_ops.rs | 1 + src/app.rs | 7 +- src/cli/memory.rs | 8 +- src/cli/mod.rs | 5 +- src/config/embeddings.rs | 47 ++- src/config/mod.rs | 2 +- src/workspace/README.md | 7 +- src/workspace/embedding_cache.rs | 613 +++++++++++++++++++++++++++++++ src/workspace/mod.rs | 28 ++ tests/workspace_integration.rs | 2 +- 10 files changed, 706 insertions(+), 14 deletions(-) create mode 100644 src/workspace/embedding_cache.rs diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index 2b489a7a..e8b8d09a 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -1961,6 +1961,7 @@ mod tests { context_messages: vec![], deferred_tool_calls: vec![], user_timezone: None, + allow_always: false, }; thread.await_approval(pending); diff --git a/src/app.rs b/src/app.rs index fa6675bf..c6892477 100644 --- a/src/app.rs +++ b/src/app.rs @@ -25,7 +25,7 @@ use crate::tools::ToolRegistry; use crate::tools::mcp::{McpProcessManager, McpSessionManager}; use crate::tools::wasm::SharedCredentialRegistry; use crate::tools::wasm::WasmToolRuntime; -use crate::workspace::{EmbeddingProvider, Workspace}; +use crate::workspace::{EmbeddingCacheConfig, EmbeddingProvider, Workspace}; /// Fully initialized application components, ready for channel wiring /// and agent construction. @@ -313,10 +313,13 @@ impl AppBuilder { // Register memory tools if database is available let workspace = if let Some(ref db) = self.db { + let emb_cache_config = EmbeddingCacheConfig { + max_entries: self.config.embeddings.cache_size, + }; let mut ws = Workspace::new_with_db(&self.config.owner_id, db.clone()) .with_search_config(&self.config.search); if let Some(ref emb) = embeddings { - ws = ws.with_embeddings(emb.clone()); + ws = ws.with_embeddings_cached(emb.clone(), emb_cache_config); } let ws = Arc::new(ws); tools.register_memory_tools(Arc::clone(&ws)); diff --git a/src/cli/memory.rs b/src/cli/memory.rs index a3df3625..2d0606a8 100644 --- a/src/cli/memory.rs +++ b/src/cli/memory.rs @@ -7,17 +7,18 @@ use std::sync::Arc; use clap::Subcommand; -use crate::workspace::{EmbeddingProvider, SearchConfig, Workspace}; +use crate::workspace::{EmbeddingCacheConfig, EmbeddingProvider, SearchConfig, Workspace}; /// Run a memory command using the Database trait (works with any backend). pub async fn run_memory_command_with_db( cmd: MemoryCommand, db: std::sync::Arc, embeddings: Option>, + cache_config: EmbeddingCacheConfig, ) -> anyhow::Result<()> { let mut workspace = Workspace::new_with_db("default", db); if let Some(emb) = embeddings { - workspace = workspace.with_embeddings(emb); + workspace = workspace.with_embeddings_cached(emb, cache_config); } match cmd { @@ -85,10 +86,11 @@ pub async fn run_memory_command( cmd: MemoryCommand, pool: deadpool_postgres::Pool, embeddings: Option>, + cache_config: EmbeddingCacheConfig, ) -> anyhow::Result<()> { let mut workspace = Workspace::new("default", pool); if let Some(emb) = embeddings { - workspace = workspace.with_embeddings(emb); + workspace = workspace.with_embeddings_cached(emb, cache_config); } match cmd { diff --git a/src/cli/mod.rs b/src/cli/mod.rs index cf3c793e..54779ae1 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -336,7 +336,10 @@ pub async fn run_memory_command(mem_cmd: &MemoryCommand) -> anyhow::Result<()> { .await .map_err(|e| anyhow::anyhow!("{}", e))?; - run_memory_command_with_db(mem_cmd.clone(), db, embeddings).await + let cache_config = crate::workspace::EmbeddingCacheConfig { + max_entries: config.embeddings.cache_size, + }; + run_memory_command_with_db(mem_cmd.clone(), db, embeddings, cache_config).await } #[cfg(test)] diff --git a/src/config/embeddings.rs b/src/config/embeddings.rs index a1c3ecd7..43fea73a 100644 --- a/src/config/embeddings.rs +++ b/src/config/embeddings.rs @@ -8,6 +8,9 @@ use crate::llm::SessionManager; use crate::settings::Settings; use crate::workspace::EmbeddingProvider; +/// Default maximum number of cached embeddings. +pub const DEFAULT_EMBEDDING_CACHE_SIZE: usize = 10_000; + /// Embeddings provider configuration. #[derive(Debug, Clone)] pub struct EmbeddingsConfig { @@ -26,6 +29,12 @@ pub struct EmbeddingsConfig { /// Custom base URL for OpenAI-compatible embedding providers. /// When set, overrides the default `https://api.openai.com`. pub openai_base_url: Option, + /// Maximum entries in the embedding LRU cache (default 10,000). + /// + /// Approximate raw embedding payload: `cache_size × dimension × 4 bytes`. + /// 10,000 × 1536 floats ≈ 58 MB (payload only; actual memory is higher + /// due to HashMap buckets, per-entry Vec/timestamp overhead). + pub cache_size: usize, } impl Default for EmbeddingsConfig { @@ -40,6 +49,7 @@ impl Default for EmbeddingsConfig { ollama_base_url: "http://localhost:11434".to_string(), dimension, openai_base_url: None, + cache_size: DEFAULT_EMBEDDING_CACHE_SIZE, } } } @@ -80,6 +90,15 @@ impl EmbeddingsConfig { let openai_base_url = optional_env("EMBEDDING_BASE_URL")?; + let cache_size = parse_optional_env("EMBEDDING_CACHE_SIZE", DEFAULT_EMBEDDING_CACHE_SIZE)?; + + if cache_size == 0 { + return Err(ConfigError::InvalidValue { + key: "EMBEDDING_CACHE_SIZE".to_string(), + message: "must be at least 1".to_string(), + }); + } + Ok(Self { enabled, provider, @@ -88,6 +107,7 @@ impl EmbeddingsConfig { ollama_base_url, dimension, openai_base_url, + cache_size, }) } @@ -183,13 +203,13 @@ mod tests { std::env::remove_var("EMBEDDING_MODEL"); std::env::remove_var("OPENAI_API_KEY"); std::env::remove_var("EMBEDDING_BASE_URL"); + std::env::remove_var("EMBEDDING_CACHE_SIZE"); } } #[test] fn embeddings_disabled_not_overridden_by_openai_key() { let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); - clear_embedding_env(); // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { @@ -240,7 +260,6 @@ mod tests { #[test] fn embeddings_env_override_takes_precedence() { let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); - clear_embedding_env(); // SAFETY: Under ENV_MUTEX. unsafe { @@ -281,10 +300,8 @@ mod tests { let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed"); assert_eq!( config.openai_base_url.as_deref(), - Some("https://custom.example.com"), - "EMBEDDING_BASE_URL env var should be parsed into openai_base_url" + Some("https://custom.example.com") ); - // SAFETY: Under ENV_MUTEX. unsafe { std::env::remove_var("EMBEDDING_BASE_URL"); @@ -303,4 +320,24 @@ mod tests { "openai_base_url should be None when EMBEDDING_BASE_URL is not set" ); } + + #[test] + fn cache_size_zero_rejected() { + let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); + clear_embedding_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::set_var("EMBEDDING_CACHE_SIZE", "0"); + } + + let settings = Settings::default(); + let result = EmbeddingsConfig::resolve(&settings); + assert!(result.is_err(), "cache_size=0 should be rejected"); + let err = result.unwrap_err().to_string(); + assert!(err.contains("at least 1"), "should mention minimum: {err}"); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("EMBEDDING_CACHE_SIZE"); + } + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 38c80880..300fb08e 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -38,7 +38,7 @@ pub use self::channels::{ ChannelsConfig, CliConfig, DEFAULT_GATEWAY_PORT, GatewayConfig, HttpConfig, SignalConfig, }; pub use self::database::{DatabaseBackend, DatabaseConfig, SslMode, default_libsql_path}; -pub use self::embeddings::EmbeddingsConfig; +pub use self::embeddings::{DEFAULT_EMBEDDING_CACHE_SIZE, EmbeddingsConfig}; pub use self::heartbeat::HeartbeatConfig; pub use self::hygiene::HygieneConfig; pub use self::llm::default_session_path; diff --git a/src/workspace/README.md b/src/workspace/README.md index 2b3ee5b4..db65294d 100644 --- a/src/workspace/README.md +++ b/src/workspace/README.md @@ -38,12 +38,17 @@ workspace/ ## Using the Workspace ```rust +use std::sync::Arc; use crate::workspace::{Workspace, OpenAiEmbeddings, paths}; -// Create workspace for a user +// Create workspace for a user (wraps embeddings in a default LRU cache) let workspace = Workspace::new("user_123", pool) .with_embeddings(Arc::new(OpenAiEmbeddings::new(api_key))); +// For tests: skip the cache layer (avoids unnecessary overhead with mocks) +// let workspace = Workspace::new("user_123", pool) +// .with_embeddings_uncached(Arc::new(MockEmbeddings::new(1536))); + // Read/write any path let doc = workspace.read("projects/alpha/notes.md").await?; workspace.write("context/priorities.md", "# Priorities\n\n1. Feature X").await?; diff --git a/src/workspace/embedding_cache.rs b/src/workspace/embedding_cache.rs new file mode 100644 index 00000000..848bd2e5 --- /dev/null +++ b/src/workspace/embedding_cache.rs @@ -0,0 +1,613 @@ +//! LRU embedding cache wrapping any [`EmbeddingProvider`]. +//! +//! Avoids redundant HTTP calls for identical texts by caching embeddings +//! in memory keyed by `SHA-256(model_name + "\0" + text)`. +//! +//! Follows the same cache pattern as `llm::response_cache::CachedProvider`: +//! `HashMap` + `last_accessed` tracking + manual LRU eviction. + +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use std::time::Instant; + +use async_trait::async_trait; +use sha2::{Digest, Sha256}; + +use crate::workspace::embeddings::{EmbeddingError, EmbeddingProvider}; + +/// Configuration for the embedding cache. +#[derive(Debug, Clone)] +pub struct EmbeddingCacheConfig { + /// Maximum number of cached embeddings (default 10,000). + /// + /// Approximate raw embedding payload: `max_entries × dimension × 4 bytes`. + /// At 10,000 entries × 1536 floats ≈ 58 MB (payload only; actual memory + /// is higher due to HashMap buckets, `[u8; 32]` hash keys, `Vec`/`Instant` + /// per-entry overhead). + pub max_entries: usize, +} + +impl Default for EmbeddingCacheConfig { + fn default() -> Self { + Self { + max_entries: crate::config::DEFAULT_EMBEDDING_CACHE_SIZE, + } + } +} + +struct CacheEntry { + embedding: Vec, + last_accessed: Instant, +} + +/// Embedding provider wrapper that caches results in memory. +/// +/// Thread-safe via `std::sync::Mutex`. The lock is **never held** +/// across `.await` points (all critical sections are scoped blocks), +/// so a synchronous mutex is cheaper than `tokio::sync::Mutex`. +pub struct CachedEmbeddingProvider { + inner: Arc, + cache: Mutex>, + config: EmbeddingCacheConfig, +} + +impl CachedEmbeddingProvider { + /// Wrap a provider with LRU caching. + /// + /// `config.max_entries` is clamped to at least 1. + pub fn new(inner: Arc, config: EmbeddingCacheConfig) -> Self { + let config = EmbeddingCacheConfig { + max_entries: config.max_entries.max(1), + }; + if config.max_entries > 100_000 { + tracing::warn!( + max_entries = config.max_entries, + "Embedding cache size exceeds 100,000 entries; memory usage may be significant" + ); + } + Self { + inner, + cache: Mutex::new(HashMap::with_capacity(config.max_entries.min(1024))), + config, + } + } + + /// Number of entries currently in the cache. + pub fn len(&self) -> usize { + self.cache.lock().unwrap_or_else(|e| e.into_inner()).len() + } + + /// Whether the cache is empty. + pub fn is_empty(&self) -> bool { + self.cache + .lock() + .unwrap_or_else(|e| e.into_inner()) + .is_empty() + } + + /// Clear all cached entries. + pub fn clear(&self) { + self.cache.lock().unwrap_or_else(|e| e.into_inner()).clear(); + } + + /// Build a deterministic cache key: `SHA-256(model_name + "\0" + text)`. + /// + /// Returns raw 32-byte hash to avoid a 64-char hex String allocation per lookup. + fn cache_key(&self, text: &str) -> [u8; 32] { + let mut hasher = Sha256::new(); + hasher.update(self.inner.model_name().as_bytes()); + hasher.update(b"\0"); + hasher.update(text.as_bytes()); + hasher.finalize().into() + } + + /// Evict the least-recently-used entry if at capacity (single-entry path). + // TODO: O(n) scan per eviction. If max_entries grows large, switch to + // an ordered data structure (e.g. `IndexMap` with swap_remove, or a + // linked-list LRU like the `lru` crate). + fn evict_lru(cache: &mut HashMap<[u8; 32], CacheEntry>, max_entries: usize) { + while cache.len() >= max_entries { + let oldest_key = cache + .iter() + .min_by_key(|(_, entry)| entry.last_accessed) + .map(|(k, _)| *k); + + if let Some(k) = oldest_key { + cache.remove(&k); + } else { + break; + } + } + } + + /// Evict the `k` oldest entries in O(n) average time via partial selection. + /// + /// Used by `embed_batch` to avoid the O(n×m) cost of calling + /// `evict_lru` per insert. + fn evict_k_oldest(cache: &mut HashMap<[u8; 32], CacheEntry>, k: usize) { + if k == 0 || cache.is_empty() { + return; + } + if k >= cache.len() { + cache.clear(); + return; + } + // Partial selection: find the k oldest in O(n) average via + // select_nth_unstable_by_key, then remove the first k entries. + let mut entries: Vec<([u8; 32], Instant)> = cache + .iter() + .map(|(key, entry)| (*key, entry.last_accessed)) + .collect(); + entries.select_nth_unstable_by_key(k - 1, |(_, t)| *t); + for (key, _) in entries.into_iter().take(k) { + cache.remove(&key); + } + } +} + +#[async_trait] +impl EmbeddingProvider for CachedEmbeddingProvider { + fn dimension(&self) -> usize { + self.inner.dimension() + } + + fn model_name(&self) -> &str { + self.inner.model_name() + } + + fn max_input_length(&self) -> usize { + self.inner.max_input_length() + } + + async fn embed(&self, text: &str) -> Result, EmbeddingError> { + let key = self.cache_key(text); + + // Check cache (short critical section) + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(entry) = guard.get_mut(&key) { + entry.last_accessed = Instant::now(); + tracing::trace!("embedding cache hit"); + return Ok(entry.embedding.clone()); + } + } + // Lock released before HTTP call. + // NOTE: Thundering herd — multiple concurrent callers with the same + // uncached key will each call the inner provider. This is acceptable: + // embeddings are idempotent and the last writer wins in the HashMap. + + let embedding = self.inner.embed(text).await?; + + // Store result. Re-check under lock: another concurrent caller may + // have inserted this key while the lock was released for the HTTP call. + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(entry) = guard.get_mut(&key) { + // Key already present (thundering herd) — just update, no eviction needed. + entry.embedding = embedding.clone(); + entry.last_accessed = Instant::now(); + } else { + Self::evict_lru(&mut guard, self.config.max_entries); + guard.insert( + key, + CacheEntry { + embedding: embedding.clone(), + last_accessed: Instant::now(), + }, + ); + } + } + + tracing::trace!("embedding cache miss"); + Ok(embedding) + } + + async fn embed_batch(&self, texts: &[String]) -> Result>, EmbeddingError> { + if texts.is_empty() { + return Ok(Vec::new()); + } + + // Partition into hits and misses + let keys: Vec<[u8; 32]> = texts.iter().map(|t| self.cache_key(t)).collect(); + let mut results: Vec>> = vec![None; texts.len()]; + let mut miss_indices: Vec = Vec::new(); + + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + let now = Instant::now(); + for (i, key) in keys.iter().enumerate() { + if let Some(entry) = guard.get_mut(key) { + entry.last_accessed = now; + results[i] = Some(entry.embedding.clone()); + } else { + miss_indices.push(i); + } + } + } + // Lock released before HTTP call + + if miss_indices.is_empty() { + tracing::trace!(count = texts.len(), "embedding batch: all cache hits"); + // All slots populated from cache hits + return results + .into_iter() + .enumerate() + .map(|(i, slot)| { + slot.ok_or_else(|| { + EmbeddingError::InvalidResponse(format!( + "embedding slot {i} was not populated" + )) + }) + }) + .collect::, _>>(); + } + + // Fetch missing embeddings + let miss_texts: Vec = miss_indices.iter().map(|&i| texts[i].clone()).collect(); + let new_embeddings = self.inner.embed_batch(&miss_texts).await?; + + if new_embeddings.len() != miss_indices.len() { + return Err(EmbeddingError::InvalidResponse(format!( + "embed_batch returned {} embeddings, expected {}", + new_embeddings.len(), + miss_indices.len() + ))); + } + + tracing::trace!( + hits = texts.len() - miss_indices.len(), + misses = miss_indices.len(), + "embedding batch: partial cache" + ); + + // Assemble results first (all misses, regardless of cache capacity). + for (orig_idx, emb) in miss_indices.iter().copied().zip(&new_embeddings) { + results[orig_idx] = Some(emb.clone()); + } + + // Cache the new embeddings, respecting max_entries. + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + // When misses exceed capacity, clear and only cache the tail. + let cacheable = miss_indices.len().min(self.config.max_entries); + let skip = miss_indices.len() - cacheable; + let need_to_evict = (guard.len() + cacheable).saturating_sub(self.config.max_entries); + if need_to_evict > 0 { + Self::evict_k_oldest(&mut guard, need_to_evict); + } + let now = Instant::now(); + for (&orig_idx, emb) in miss_indices[skip..].iter().zip(&new_embeddings[skip..]) { + guard.insert( + keys[orig_idx], + CacheEntry { + embedding: emb.clone(), + last_accessed: now, + }, + ); + } + } + + results + .into_iter() + .enumerate() + .map(|(i, slot)| { + slot.ok_or_else(|| { + EmbeddingError::InvalidResponse(format!("embedding slot {i} was not populated")) + }) + }) + .collect() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicU32, Ordering}; + + /// Mock embedding provider that counts calls. + struct CountingMock { + dimension: usize, + model: String, + embed_calls: AtomicU32, + batch_calls: AtomicU32, + } + + impl CountingMock { + fn new(dimension: usize, model: &str) -> Self { + Self { + dimension, + model: model.to_string(), + embed_calls: AtomicU32::new(0), + batch_calls: AtomicU32::new(0), + } + } + + fn embed_calls(&self) -> u32 { + self.embed_calls.load(Ordering::SeqCst) + } + + fn batch_calls(&self) -> u32 { + self.batch_calls.load(Ordering::SeqCst) + } + } + + #[async_trait] + impl EmbeddingProvider for CountingMock { + fn dimension(&self) -> usize { + self.dimension + } + fn model_name(&self) -> &str { + &self.model + } + fn max_input_length(&self) -> usize { + 10_000 + } + async fn embed(&self, text: &str) -> Result, EmbeddingError> { + self.embed_calls.fetch_add(1, Ordering::SeqCst); + // Simple deterministic embedding: val = text.len() / 100.0 + let val = text.len() as f32 / 100.0; + Ok(vec![val; self.dimension]) + } + async fn embed_batch(&self, texts: &[String]) -> Result>, EmbeddingError> { + self.batch_calls.fetch_add(1, Ordering::SeqCst); + texts + .iter() + .map(|t| { + let val = t.len() as f32 / 100.0; + Ok(vec![val; self.dimension]) + }) + .collect() + } + } + + #[tokio::test] + async fn cache_hit_avoids_inner_call() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + let r1 = cached.embed("hello").await.unwrap(); + assert_eq!(inner.embed_calls(), 1); + + let r2 = cached.embed("hello").await.unwrap(); + assert_eq!(inner.embed_calls(), 1); // still 1 -- cache hit + assert_eq!(r1, r2); + + assert_eq!(cached.len(), 1); + } + + #[tokio::test] + async fn cache_miss_calls_inner() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + cached.embed("hello").await.unwrap(); + cached.embed("world").await.unwrap(); + assert_eq!(inner.embed_calls(), 2); + assert_eq!(cached.len(), 2); + } + + #[tokio::test] + async fn cache_key_includes_model() { + let inner_a = Arc::new(CountingMock::new(4, "model-a")); + let inner_b = Arc::new(CountingMock::new(4, "model-b")); + + let cached_a = CachedEmbeddingProvider::new( + inner_a.clone(), + EmbeddingCacheConfig { max_entries: 100 }, + ); + let cached_b = CachedEmbeddingProvider::new( + inner_b.clone(), + EmbeddingCacheConfig { max_entries: 100 }, + ); + + // Same text, different models -> different cache keys + let key_a = cached_a.cache_key("hello"); + let key_b = cached_b.cache_key("hello"); + assert_ne!(key_a, key_b); + } + + #[tokio::test] + async fn lru_eviction() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 2 }); + + cached.embed("first").await.unwrap(); + cached.embed("second").await.unwrap(); + assert_eq!(cached.len(), 2); + + // Third entry should evict the oldest ("first") + cached.embed("third").await.unwrap(); + assert_eq!(cached.len(), 2); + assert_eq!(inner.embed_calls(), 3); + + // "first" should be a cache miss now + cached.embed("first").await.unwrap(); + assert_eq!(inner.embed_calls(), 4); + } + + #[tokio::test] + async fn embed_batch_partial_hits() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + // Pre-cache one text + cached.embed("cached").await.unwrap(); + assert_eq!(inner.embed_calls(), 1); + + // Batch with 1 cached + 2 new + let texts = vec![ + "cached".to_string(), + "new_one".to_string(), + "new_two".to_string(), + ]; + let results = cached.embed_batch(&texts).await.unwrap(); + + // Should have called embed_batch on inner for 2 misses + assert_eq!(inner.batch_calls(), 1); + assert_eq!(results.len(), 3); + assert_eq!(cached.len(), 3); + } + + #[tokio::test] + async fn batch_preserves_order() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + // Pre-cache "bb" (len 2) + cached.embed("bb").await.unwrap(); + + // Batch: "a" (miss, len 1), "bb" (hit, len 2), "ccc" (miss, len 3) + let texts = vec!["a".to_string(), "bb".to_string(), "ccc".to_string()]; + let results = cached.embed_batch(&texts).await.unwrap(); + + assert_eq!(results.len(), 3); + let expected_a = vec![1.0_f32 / 100.0; 4]; + let expected_bb = vec![2.0_f32 / 100.0; 4]; + let expected_ccc = vec![3.0_f32 / 100.0; 4]; + assert_eq!(results[0], expected_a); + assert_eq!(results[1], expected_bb); + assert_eq!(results[2], expected_ccc); + } + + #[tokio::test] + async fn batch_exceeding_capacity_respects_max_entries() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 3 }); + + // Batch with 5 misses but cache capacity is 3 + let texts: Vec = (0..5).map(|i| format!("text_{i}")).collect(); + let results = cached.embed_batch(&texts).await.unwrap(); + + assert_eq!(results.len(), 5); + let len = cached.len(); + assert!(len <= 3, "cache len {len} exceeds max 3"); + } + + /// Mock embedding provider that fails the first N calls, then succeeds. + struct FailThenSucceedMock { + dimension: usize, + model: String, + remaining_failures: AtomicU32, + } + + impl FailThenSucceedMock { + fn new(dimension: usize, fail_count: u32) -> Self { + Self { + dimension, + model: "fail-mock".to_string(), + remaining_failures: AtomicU32::new(fail_count), + } + } + } + + #[async_trait] + impl EmbeddingProvider for FailThenSucceedMock { + fn dimension(&self) -> usize { + self.dimension + } + fn model_name(&self) -> &str { + &self.model + } + fn max_input_length(&self) -> usize { + 10_000 + } + async fn embed(&self, text: &str) -> Result, EmbeddingError> { + let prev = + self.remaining_failures + .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |v| { + if v > 0 { Some(v - 1) } else { None } + }); + if prev.is_ok() { + return Err(EmbeddingError::HttpError("simulated failure".to_string())); + } + let val = text.len() as f32 / 100.0; + Ok(vec![val; self.dimension]) + } + async fn embed_batch(&self, texts: &[String]) -> Result>, EmbeddingError> { + let prev = + self.remaining_failures + .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |v| { + if v > 0 { Some(v - 1) } else { None } + }); + if prev.is_ok() { + return Err(EmbeddingError::HttpError("simulated failure".to_string())); + } + texts + .iter() + .map(|t| { + let val = t.len() as f32 / 100.0; + Ok(vec![val; self.dimension]) + }) + .collect() + } + } + + #[tokio::test] + async fn error_does_not_pollute_cache() { + let inner = Arc::new(FailThenSucceedMock::new(4, 1)); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + // First call fails + let err = cached.embed("hello").await; + assert!(err.is_err()); + assert!(cached.is_empty(), "cache should be empty after error"); + + // Second call succeeds and should call the inner provider (not serve stale error) + let result = cached.embed("hello").await; + assert!(result.is_ok()); + assert_eq!(cached.len(), 1); + } + + #[tokio::test] + async fn embed_batch_empty_input() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + let results = cached.embed_batch(&[]).await.unwrap(); + assert!(results.is_empty()); + assert_eq!(inner.batch_calls(), 0); + } + + #[tokio::test] + async fn embed_batch_all_misses() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 100 }); + + // Nothing cached — every text is a miss + let texts: Vec = vec!["alpha".into(), "beta".into(), "gamma".into()]; + let results = cached.embed_batch(&texts).await.unwrap(); + assert_eq!(results.len(), 3); + assert_eq!(inner.batch_calls(), 1, "inner called once for misses"); + assert_eq!(cached.len(), 3, "all results should be cached"); + + // Second call should be all hits — no new inner calls + let results2 = cached.embed_batch(&texts).await.unwrap(); + assert_eq!(results2.len(), 3); + assert_eq!(inner.batch_calls(), 1, "no new inner calls"); + } + + #[tokio::test] + async fn zero_max_entries_clamped_to_one() { + let inner = Arc::new(CountingMock::new(4, "test-model")); + let cached = + CachedEmbeddingProvider::new(inner.clone(), EmbeddingCacheConfig { max_entries: 0 }); + + // Should behave as max_entries=1 (clamped in constructor) + cached.embed("hello").await.unwrap(); + assert_eq!(cached.len(), 1); + + // Second entry evicts the first + cached.embed("world").await.unwrap(); + assert_eq!(cached.len(), 1); + assert_eq!(inner.embed_calls(), 2); + } +} diff --git a/src/workspace/mod.rs b/src/workspace/mod.rs index ad233caf..f2a59809 100644 --- a/src/workspace/mod.rs +++ b/src/workspace/mod.rs @@ -42,6 +42,7 @@ mod chunker; mod document; +mod embedding_cache; mod embeddings; pub mod hygiene; #[cfg(feature = "postgres")] @@ -50,6 +51,7 @@ mod search; pub use chunker::{ChunkConfig, chunk_document}; pub use document::{MemoryChunk, MemoryDocument, WorkspaceEntry, paths}; +pub use embedding_cache::{CachedEmbeddingProvider, EmbeddingCacheConfig}; pub use embeddings::{ EmbeddingProvider, MockEmbeddings, NearAiEmbeddings, OllamaEmbeddings, OpenAiEmbeddings, }; @@ -371,7 +373,33 @@ impl Workspace { } /// Set the embedding provider for semantic search. + /// + /// The provider is automatically wrapped in a [`CachedEmbeddingProvider`] + /// with the default cache size (10,000 entries; payload ~58 MB for 1536-dim, + /// actual memory higher due to per-entry overhead). pub fn with_embeddings(mut self, provider: Arc) -> Self { + self.embeddings = Some(Arc::new(CachedEmbeddingProvider::new( + provider, + EmbeddingCacheConfig::default(), + ))); + self + } + + /// Set the embedding provider with a custom cache configuration. + pub fn with_embeddings_cached( + mut self, + provider: Arc, + cache_config: EmbeddingCacheConfig, + ) -> Self { + self.embeddings = Some(Arc::new(CachedEmbeddingProvider::new( + provider, + cache_config, + ))); + self + } + + /// Set the embedding provider **without** caching (for tests). + pub fn with_embeddings_uncached(mut self, provider: Arc) -> Self { self.embeddings = Some(provider); self } diff --git a/tests/workspace_integration.rs b/tests/workspace_integration.rs index dddd95e9..2182fc38 100644 --- a/tests/workspace_integration.rs +++ b/tests/workspace_integration.rs @@ -308,7 +308,7 @@ async fn test_workspace_hybrid_search_with_mock_embeddings() { // Create workspace with mock embeddings (1536 dimensions to match OpenAI) let embeddings = Arc::new(MockEmbeddings::new(1536)); - let workspace = Workspace::new(user_id, pool.clone()).with_embeddings(embeddings); + let workspace = Workspace::new(user_id, pool.clone()).with_embeddings_uncached(embeddings); // Write documents workspace From 65062f3cc069ebbd29f6d9be874ae5eff796e43a Mon Sep 17 00:00:00 2001 From: alexthebuildr <116134064+ztsalexey@users.noreply.github.com> Date: Thu, 19 Mar 2026 14:43:04 -0600 Subject: [PATCH 2/3] feat: structured fallback deliverables for failed/stuck jobs (#236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: structured fallback deliverables for failed/stuck jobs (#221) When a job fails or gets stuck, build a FallbackDeliverable that captures partial results, action statistics, cost, timing, and repair attempts. This replaces opaque error strings with structured data users can act on. - Add FallbackDeliverable, LastAction, ActionStats types in context/fallback.rs - Store fallback in JobContext.metadata["fallback_deliverable"] on failure - Surface fallback in job_status tool output and SSE job_result events - Update mark_failed() and mark_stuck() in worker to build fallback - 8 unit tests covering zero/mixed actions, truncation, timing, serialization Co-Authored-By: Claude Opus 4.6 * fix: address review comments on fallback deliverables - Fix doc comment: "200 chars" -> "200 bytes (UTF-8 safe)" since truncate_str operates on byte length, not character count. - Add code comment documenting that SSE fallback_deliverable is currently always None (forward-compatible infrastructure). Co-Authored-By: Claude Opus 4.6 * refactor: take Option<&FallbackDeliverable> instead of &Option<…> Addresses Gemini review feedback: idiomatic Rust prefers Option<&T> over &Option for borrowed optional values. Co-Authored-By: Claude Opus 4.6 * fix: guard against non-object metadata and add fallback test - store_fallback_in_metadata now resets metadata to {} when it's any non-object type (string, array, number), not just null. Prevents panic on index assignment. - Add test_job_status_includes_fallback_deliverable to verify the fallback field is surfaced in job_status tool output. Co-Authored-By: Claude Opus 4.6 * fix: use sanitized output in fallback preview + add integration tests Security fix: FallbackDeliverable::build() now uses output_sanitized instead of output_raw, preventing secrets/PII from leaking through the job_status tool and SSE job_result events. Also adds: - test_fallback_uses_sanitized_output: proves raw secrets don't leak - test_store_fallback_in_metadata_roundtrip: full serialize/deserialize - test_store_fallback_handles_non_object_metadata: edge case coverage - test_store_fallback_none_is_noop: None input is safe Addresses serrrfirat review feedback on PR #236. Co-Authored-By: Claude Opus 4.6 * fix: harden fallback deliverables against review findings - Truncate failure_reason to 1000 bytes to prevent metadata bloat - Add tracing::warn on fallback serialization failure (was silently discarded) - Fix module/struct docs to cover stuck jobs, remove stale SSE claim - Fix job.rs test to use real FallbackDeliverable field names - Add tests for failure_reason truncation and completed_at=None elapsed time - Fix pre-existing clippy warning in settings.rs (field_reassign_with_default) Co-Authored-By: Claude Opus 4.6 * fix: address Copilot review findings on fallback deliverables - Fix output_raw/output_sanitized field swap in ActionRecord::succeed() so sanitized data actually goes into the sanitized field (security) - Return None instead of empty Memory when get_memory fails in build_fallback, with tracing::warn for observability - Replace manual elapsed calculation with ctx.elapsed() which already clamps negative durations Co-Authored-By: Claude Opus 4.6 * fix: resolve rebase conflicts and update tests for parameter swap - Add fallback field to SseEvent::JobResult in job_monitor - Fix type annotation in fallback deliverable test - Update test_action_record_succeed_sets_fields for new parameter order - Use create_job_for_user in test (API changed on main) Co-Authored-By: Claude Opus 4.6 * chore: trigger CI re-check after rebase * fix: fall back to error message for failed action output_preview When the last action is a failed tool call, output_sanitized is None, leaving output_preview empty. Now falls back to the action's error message so users see what went wrong. [skip-regression-check] * ci: add safety comments to test code for no-panics check The CI no-panics grep check cannot distinguish test code inside src/ files from production code. Add // safety: test annotations to .unwrap(), .expect(), and assert!() calls in #[cfg(test)] modules. * fix: clarify succeed() doc and avoid clone in output_preview - Fix doc comment: output_raw is stored as pretty-printed JSON string, not a raw JSON value - Borrow string slice directly in fallback preview to avoid cloning potentially large sanitized outputs before truncation * refactor: reuse floor_char_boundary in truncate_str Replace hand-rolled UTF-8 boundary logic with existing crate::util::floor_char_boundary to reduce duplication. * fix: rename SSE fallback field to fallback_deliverable for consistency The SSE JobResult field was named `fallback` while everywhere else (metadata key, job_status tool) uses `fallback_deliverable`. Align the SSE wire format to avoid forcing clients to handle two names. --------- Co-authored-by: Claude Opus 4.6 --- src/agent/job_monitor.rs | 1 + src/channels/web/types.rs | 2 + src/context/fallback.rs | 319 ++++++++++++++++++++++++++++++++++++++ src/context/memory.rs | 149 +++++++++--------- src/context/mod.rs | 2 + src/orchestrator/api.rs | 6 + src/tools/builtin/job.rs | 300 +++++++++++++++++++++-------------- src/worker/job.rs | 177 +++++++++++++++++---- 8 files changed, 739 insertions(+), 217 deletions(-) create mode 100644 src/context/fallback.rs diff --git a/src/agent/job_monitor.rs b/src/agent/job_monitor.rs index 714caeac..6497861a 100644 --- a/src/agent/job_monitor.rs +++ b/src/agent/job_monitor.rs @@ -211,6 +211,7 @@ mod tests { job_id: job_id.to_string(), status: "completed".to_string(), session_id: None, + fallback_deliverable: None, }, )) .unwrap(); diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index b2c060c9..861b5bd2 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -232,6 +232,8 @@ pub enum SseEvent { status: String, #[serde(skip_serializing_if = "Option::is_none")] session_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + fallback_deliverable: Option, }, /// An image was generated by a tool. diff --git a/src/context/fallback.rs b/src/context/fallback.rs new file mode 100644 index 00000000..6e765573 --- /dev/null +++ b/src/context/fallback.rs @@ -0,0 +1,319 @@ +//! Structured fallback deliverables for failed or stuck jobs. +//! +//! When a job fails or is detected as stuck, a [`FallbackDeliverable`] captures +//! what was accomplished before the failure: partial results, action statistics, +//! cost, and timing. This gives users visibility into terminal jobs instead of +//! just an error string. +//! +//! Fallback deliverables are stored in `JobContext.metadata["fallback_deliverable"]` +//! and surfaced through the `job_status` tool. + +use serde::{Deserialize, Serialize}; + +use crate::context::memory::Memory; +use crate::context::state::JobContext; + +/// Structured summary of a failed or stuck job. +/// +/// Stored in `JobContext.metadata["fallback_deliverable"]` when a job fails +/// or is marked stuck. Surfaced through the `job_status` tool. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct FallbackDeliverable { + /// True if at least one action succeeded before failure. + pub partial: bool, + /// Why the job failed. + pub failure_reason: String, + /// Last action taken before failure. + pub last_action: Option, + /// Aggregate action statistics. + pub action_stats: ActionStats, + /// Total tokens consumed. + pub tokens_used: u64, + /// Total cost incurred (decimal as string for JSON safety). + pub cost: String, + /// Wall-clock elapsed time in seconds. + pub elapsed_secs: f64, + /// Number of self-repair attempts. + pub repair_attempts: u32, +} + +/// Summary of the last action taken before failure. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct LastAction { + pub tool_name: String, + /// Truncated to 200 bytes (UTF-8 safe). + pub output_preview: String, + pub success: bool, +} + +/// Aggregate action counts. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ActionStats { + pub total: u32, + pub successful: u32, + pub failed: u32, +} + +impl FallbackDeliverable { + /// Build a fallback deliverable from a job context and its memory. + pub fn build(ctx: &JobContext, memory: &Memory, reason: &str) -> Self { + let successful = memory.successful_actions() as u32; + let failed = memory.failed_actions() as u32; + let total = memory.actions.len() as u32; + + let last_action = memory.last_action().map(|a| { + // Use sanitized output to avoid leaking secrets through the fallback API surface. + // For failed actions (no sanitized output), fall back to the error message. + // Borrow the string slice directly when possible to avoid cloning + // potentially large outputs just for truncation. + let owned_fallback; + let preview_str: &str = if let Some(v) = a.output_sanitized.as_ref() { + match v { + serde_json::Value::String(s) => s.as_str(), + other => { + owned_fallback = serde_json::to_string(other).unwrap_or_default(); + &owned_fallback + } + } + } else if let Some(ref err) = a.error { + err.as_str() + } else { + "" + }; + let preview = truncate_str(preview_str, 200); + LastAction { + tool_name: a.tool_name.clone(), + output_preview: preview.to_string(), + success: a.success, + } + }); + + let elapsed_secs = ctx.elapsed().map_or(0.0, |d| d.as_secs_f64()); + + Self { + partial: successful > 0, + failure_reason: truncate_str(reason, 1000).to_string(), + last_action, + action_stats: ActionStats { + total, + successful, + failed, + }, + tokens_used: ctx.total_tokens_used, + cost: ctx.actual_cost.to_string(), + elapsed_secs, + repair_attempts: ctx.repair_attempts, + } + } +} + +/// Truncate a string to at most `max_len` bytes on a char boundary. +fn truncate_str(s: &str, max_len: usize) -> &str { + &s[..crate::util::floor_char_boundary(s, max_len)] +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::context::memory::Memory; + use crate::context::state::JobContext; + use chrono::{Duration, Utc}; + use rust_decimal::Decimal; + use std::time::Duration as StdDuration; + + #[test] + fn test_fallback_zero_actions() { + let ctx = JobContext::new("Test", "Empty job"); + let memory = Memory::new(ctx.job_id); + + let fb = FallbackDeliverable::build(&ctx, &memory, "timed out"); + + assert!(!fb.partial); // safety: test + assert_eq!(fb.failure_reason, "timed out"); // safety: test + assert!(fb.last_action.is_none()); // safety: test + assert_eq!(fb.action_stats.total, 0); // safety: test + assert_eq!(fb.action_stats.successful, 0); // safety: test + assert_eq!(fb.action_stats.failed, 0); // safety: test + assert_eq!(fb.tokens_used, 0); // safety: test + assert_eq!(fb.cost, "0"); // safety: test + assert_eq!(fb.repair_attempts, 0); // safety: test + } + + #[test] + fn test_fallback_mixed_actions() { + let mut ctx = JobContext::new("Test", "Mixed job"); + ctx.total_tokens_used = 5000; + ctx.actual_cost = Decimal::new(42, 2); // 0.42 + ctx.repair_attempts = 1; + + let mut memory = Memory::new(ctx.job_id); + + // 3 successes + for _ in 0..3 { + let action = memory + .create_action("tool_a", serde_json::json!({})) + .succeed( + Some("output".to_string()), + serde_json::json!({}), + StdDuration::from_secs(1), + ); + memory.record_action(action); + } + // 2 failures + for _ in 0..2 { + let action = memory + .create_action("tool_b", serde_json::json!({})) + .fail("broke", StdDuration::from_secs(1)); + memory.record_action(action); + } + + let fb = FallbackDeliverable::build(&ctx, &memory, "max iterations"); + + assert!(fb.partial); // safety: test + assert_eq!(fb.action_stats.total, 5); // safety: test + assert_eq!(fb.action_stats.successful, 3); // safety: test + assert_eq!(fb.action_stats.failed, 2); // safety: test + assert_eq!(fb.tokens_used, 5000); // safety: test + assert_eq!(fb.cost, "0.42"); // safety: test + assert_eq!(fb.repair_attempts, 1); // safety: test + assert!(fb.last_action.is_some()); // safety: test + let la = fb.last_action.unwrap(); // safety: test + assert_eq!(la.tool_name, "tool_b"); // safety: test + assert!(!la.success); // safety: test + // Failed actions should surface the error message as the output preview + assert_eq!(la.output_preview, "broke"); // safety: test + } + + #[test] + fn test_fallback_failed_action_shows_error() { + let ctx = JobContext::new("Test", "Error preview"); + let mut memory = Memory::new(ctx.job_id); + + let action = memory + .create_action("broken_tool", serde_json::json!({})) + .fail("connection timed out after 30s", StdDuration::from_secs(30)); + memory.record_action(action); + + let fb = FallbackDeliverable::build(&ctx, &memory, "tool failure"); + let la = fb.last_action.unwrap(); // safety: test + assert!(!la.success); // safety: test + assert_eq!(la.output_preview, "connection timed out after 30s"); // safety: test + } + + #[test] + fn test_fallback_last_action_truncation() { + let ctx = JobContext::new("Test", "Truncation"); + let mut memory = Memory::new(ctx.job_id); + + let long_output = "x".repeat(500); + let action = memory + .create_action("tool_c", serde_json::json!({})) + .succeed( + Some(long_output.clone()), + serde_json::Value::String(long_output), + StdDuration::from_secs(1), + ); + memory.record_action(action); + + let fb = FallbackDeliverable::build(&ctx, &memory, "failed"); + let la = fb.last_action.unwrap(); // safety: test + assert!(la.output_preview.len() <= 200); // safety: test + assert!(!la.output_preview.is_empty()); // safety: test + } + + #[test] + fn test_fallback_uses_sanitized_output() { + let ctx = JobContext::new("Test", "Sanitized"); + let mut memory = Memory::new(ctx.job_id); + + let action = memory + .create_action("tool_d", serde_json::json!({})) + .succeed( + Some("[REDACTED]".to_string()), + serde_json::json!({"api_key": "sk-secret-key-12345"}), + StdDuration::from_secs(1), + ); + memory.record_action(action); + + let fb = FallbackDeliverable::build(&ctx, &memory, "failed"); + let la = fb.last_action.unwrap(); // safety: test + // Must use sanitized output, not raw + assert!(!la.output_preview.contains("sk-secret")); // safety: test + assert!(la.output_preview.contains("REDACTED")); // safety: test + } + + #[test] + fn test_fallback_elapsed_time() { + let mut ctx = JobContext::new("Test", "Timing"); + let now = Utc::now(); + ctx.started_at = Some(now - Duration::seconds(10)); + ctx.completed_at = Some(now); + + let memory = Memory::new(ctx.job_id); + let fb = FallbackDeliverable::build(&ctx, &memory, "failed"); + + // Should be approximately 10 seconds + assert!((fb.elapsed_secs - 10.0).abs() < 0.1); // safety: test + } + + #[test] + fn test_fallback_no_started_at() { + let ctx = JobContext::new("Test", "Never started"); + let memory = Memory::new(ctx.job_id); + + let fb = FallbackDeliverable::build(&ctx, &memory, "failed"); + assert!((fb.elapsed_secs - 0.0).abs() < 0.001); // safety: test + } + + #[test] + fn test_fallback_elapsed_time_no_completed_at() { + let mut ctx = JobContext::new("Test", "Still running"); + ctx.started_at = Some(Utc::now() - Duration::seconds(5)); + // completed_at is None — should use Utc::now() as fallback + + let memory = Memory::new(ctx.job_id); + let fb = FallbackDeliverable::build(&ctx, &memory, "stuck"); + + // Should be approximately 5 seconds (using now as end time) + assert!(fb.elapsed_secs >= 4.0 && fb.elapsed_secs <= 7.0); // safety: test + } + + #[test] + fn test_fallback_failure_reason_truncation() { + let ctx = JobContext::new("Test", "Long reason"); + let memory = Memory::new(ctx.job_id); + + let long_reason = "x".repeat(5000); + let fb = FallbackDeliverable::build(&ctx, &memory, &long_reason); + + assert!(fb.failure_reason.len() <= 1000); // safety: test + assert!(!fb.failure_reason.is_empty()); // safety: test + } + + #[test] + fn test_truncate_str_ascii() { + assert_eq!(truncate_str("hello", 10), "hello"); // safety: test + assert_eq!(truncate_str("hello world", 5), "hello"); // safety: test + } + + #[test] + fn test_truncate_str_unicode() { + // "é" is 2 bytes in UTF-8 + let s = "café"; + assert_eq!(truncate_str(s, 10), "café"); // safety: test + // Truncating at 4 would split "é", should back up to 3 + assert_eq!(truncate_str(s, 4), "caf"); // safety: test + } + + #[test] + fn test_fallback_serialization() { + let ctx = JobContext::new("Test", "Serialize"); + let memory = Memory::new(ctx.job_id); + let fb = FallbackDeliverable::build(&ctx, &memory, "test error"); + + // Should serialize to JSON and back without error + let json = serde_json::to_value(&fb).unwrap(); // safety: test + let deserialized: FallbackDeliverable = serde_json::from_value(json).unwrap(); // safety: test + assert_eq!(deserialized.failure_reason, "test error"); // safety: test + } +} diff --git a/src/context/memory.rs b/src/context/memory.rs index 9452c649..05313e67 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -58,15 +58,19 @@ impl ActionRecord { } /// Mark the action as successful. + /// + /// `output_sanitized` is the tool output after safety processing (string). + /// `output_raw` is the original tool result (JSON value, stored as a + /// pretty-printed JSON string in `ActionRecord.output_raw`). pub fn succeed( mut self, - output_raw: Option, - output_sanitized: serde_json::Value, + output_sanitized: Option, + output_raw: serde_json::Value, duration: Duration, ) -> Self { self.success = true; - self.output_raw = output_raw; - self.output_sanitized = Some(output_sanitized); + self.output_raw = Some(serde_json::to_string_pretty(&output_raw).unwrap_or_default()); + self.output_sanitized = output_sanitized.map(serde_json::Value::String); self.duration = duration; self } @@ -248,15 +252,15 @@ mod tests { #[test] fn test_action_record() { let action = ActionRecord::new(0, "test", serde_json::json!({"key": "value"})); - assert_eq!(action.sequence, 0); - assert!(!action.success); + assert_eq!(action.sequence, 0); // safety: test + assert!(!action.success); // safety: test let action = action.succeed( Some("raw".to_string()), serde_json::json!({"result": "ok"}), Duration::from_millis(100), ); - assert!(action.success); + assert!(action.success); // safety: test } #[test] @@ -267,7 +271,7 @@ mod tests { memory.add(ChatMessage::user("How are you?")); memory.add(ChatMessage::assistant("Good!")); - assert_eq!(memory.len(), 3); // Oldest removed + assert_eq!(memory.len(), 3); // Oldest removed // safety: test } #[test] @@ -286,9 +290,9 @@ mod tests { .with_cost(Decimal::new(20, 1)); memory.record_action(action2); - assert_eq!(memory.total_cost(), Decimal::new(30, 1)); - assert_eq!(memory.total_duration(), Duration::from_secs(3)); - assert_eq!(memory.successful_actions(), 2); + assert_eq!(memory.total_cost(), Decimal::new(30, 1)); // safety: test + assert_eq!(memory.total_duration(), Duration::from_secs(3)); // safety: test + assert_eq!(memory.successful_actions(), 2); // safety: test } #[test] @@ -296,11 +300,11 @@ mod tests { let action = ActionRecord::new(1, "broken_tool", serde_json::json!({"x": 1})); let action = action.fail("something went wrong", Duration::from_millis(50)); - assert!(!action.success); - assert_eq!(action.error.as_deref(), Some("something went wrong")); - assert_eq!(action.duration, Duration::from_millis(50)); - assert!(action.output_raw.is_none()); - assert!(action.output_sanitized.is_none()); + assert!(!action.success); // safety: test + assert_eq!(action.error.as_deref(), Some("something went wrong")); // safety: test + assert_eq!(action.duration, Duration::from_millis(50)); // safety: test + assert!(action.output_raw.is_none()); // safety: test + assert!(action.output_sanitized.is_none()); // safety: test } #[test] @@ -308,9 +312,9 @@ mod tests { let action = ActionRecord::new(0, "risky_tool", serde_json::json!({})); let action = action.with_warnings(vec!["suspicious pattern".into(), "possible xss".into()]); - assert_eq!(action.sanitization_warnings.len(), 2); - assert_eq!(action.sanitization_warnings[0], "suspicious pattern"); - assert_eq!(action.sanitization_warnings[1], "possible xss"); + assert_eq!(action.sanitization_warnings.len(), 2); // safety: test + assert_eq!(action.sanitization_warnings[0], "suspicious pattern"); // safety: test + assert_eq!(action.sanitization_warnings[1], "possible xss"); // safety: test } #[test] @@ -319,41 +323,46 @@ mod tests { let cost = Decimal::new(42, 2); // 0.42 let action = action.with_cost(cost); - assert_eq!(action.cost, Some(Decimal::new(42, 2))); + assert_eq!(action.cost, Some(Decimal::new(42, 2))); // safety: test } #[test] fn test_action_record_new_defaults() { let action = ActionRecord::new(5, "my_tool", serde_json::json!({"key": "val"})); - assert_eq!(action.sequence, 5); - assert_eq!(action.tool_name, "my_tool"); - assert_eq!(action.input, serde_json::json!({"key": "val"})); - assert!(!action.success); - assert!(action.output_raw.is_none()); - assert!(action.output_sanitized.is_none()); - assert!(action.sanitization_warnings.is_empty()); - assert!(action.cost.is_none()); - assert_eq!(action.duration, Duration::ZERO); - assert!(action.error.is_none()); + assert_eq!(action.sequence, 5); // safety: test + assert_eq!(action.tool_name, "my_tool"); // safety: test + assert_eq!(action.input, serde_json::json!({"key": "val"})); // safety: test + assert!(!action.success); // safety: test + assert!(action.output_raw.is_none()); // safety: test + assert!(action.output_sanitized.is_none()); // safety: test + assert!(action.sanitization_warnings.is_empty()); // safety: test + assert!(action.cost.is_none()); // safety: test + assert_eq!(action.duration, Duration::ZERO); // safety: test + assert!(action.error.is_none()); // safety: test } #[test] fn test_action_record_succeed_sets_fields() { let action = ActionRecord::new(0, "tool", serde_json::json!({})); let action = action.succeed( - Some("raw output here".into()), + Some("sanitized output".into()), serde_json::json!({"clean": true}), Duration::from_secs(7), ); - assert!(action.success); - assert_eq!(action.output_raw.as_deref(), Some("raw output here")); + assert!(action.success); // safety: test + // output_raw is the JSON value pretty-printed + let expected_raw = + serde_json::to_string_pretty(&serde_json::json!({"clean": true})).unwrap(); // safety: test + assert_eq!(action.output_raw.as_deref(), Some(expected_raw.as_str())); // safety: test + // output_sanitized wraps the string in a JSON string value assert_eq!( + /* safety: test */ action.output_sanitized, - Some(serde_json::json!({"clean": true})) + Some(serde_json::json!("sanitized output")) ); - assert_eq!(action.duration, Duration::from_secs(7)); + assert_eq!(action.duration, Duration::from_secs(7)); // safety: test } #[test] @@ -361,13 +370,13 @@ mod tests { let mut mem = ConversationMemory::new(10); mem.add(ChatMessage::user("hello")); mem.add(ChatMessage::assistant("hi")); - assert_eq!(mem.len(), 2); - assert!(!mem.is_empty()); + assert_eq!(mem.len(), 2); // safety: test + assert!(!mem.is_empty()); // safety: test mem.clear(); - assert_eq!(mem.len(), 0); - assert!(mem.is_empty()); - assert!(mem.messages().is_empty()); + assert_eq!(mem.len(), 0); // safety: test + assert!(mem.is_empty()); // safety: test + assert!(mem.messages().is_empty()); // safety: test } #[test] @@ -379,20 +388,20 @@ mod tests { mem.add(ChatMessage::assistant("four")); let last_2 = mem.last_n(2); - assert_eq!(last_2.len(), 2); - assert_eq!(last_2[0].content, "three"); - assert_eq!(last_2[1].content, "four"); + assert_eq!(last_2.len(), 2); // safety: test + assert_eq!(last_2[0].content, "three"); // safety: test + assert_eq!(last_2[1].content, "four"); // safety: test // Requesting more than available returns all let last_100 = mem.last_n(100); - assert_eq!(last_100.len(), 4); + assert_eq!(last_100.len(), 4); // safety: test } #[test] fn test_conversation_memory_last_n_empty() { let mem = ConversationMemory::new(10); let result = mem.last_n(5); - assert!(result.is_empty()); + assert!(result.is_empty()); // safety: test } #[test] @@ -405,13 +414,13 @@ mod tests { // At capacity (3). Adding one more should trim, but keep system. mem.add(ChatMessage::user("msg3")); - assert_eq!(mem.len(), 3); + assert_eq!(mem.len(), 3); // safety: test // System message must survive - assert_eq!(mem.messages()[0].role, crate::llm::Role::System); - assert_eq!(mem.messages()[0].content, "You are helpful"); + assert_eq!(mem.messages()[0].role, crate::llm::Role::System); // safety: test + assert_eq!(mem.messages()[0].content, "You are helpful"); // safety: test // Oldest non-system message (msg1) should be gone - assert_eq!(mem.messages()[1].content, "msg2"); - assert_eq!(mem.messages()[2].content, "msg3"); + assert_eq!(mem.messages()[1].content, "msg2"); // safety: test + assert_eq!(mem.messages()[2].content, "msg3"); // safety: test } #[test] @@ -422,9 +431,9 @@ mod tests { // Now at capacity. Add another. mem.add(ChatMessage::user("b")); - assert_eq!(mem.len(), 2); - assert_eq!(mem.messages()[0].role, crate::llm::Role::System); - assert_eq!(mem.messages()[1].content, "b"); + assert_eq!(mem.len(), 2); // safety: test + assert_eq!(mem.messages()[0].role, crate::llm::Role::System); // safety: test + assert_eq!(mem.messages()[1].content, "b"); // safety: test } #[test] @@ -440,7 +449,7 @@ mod tests { mem.add(ChatMessage::user("hello")); // Should have broken out rather than looping forever. // The system message is protected, so len may exceed max. - assert!(mem.len() <= 2); + assert!(mem.len() <= 2); // safety: test } #[test] @@ -459,14 +468,14 @@ mod tests { .fail("oops", Duration::from_millis(2)); memory.record_action(err); - assert_eq!(memory.successful_actions(), 1); - assert_eq!(memory.failed_actions(), 1); + assert_eq!(memory.successful_actions(), 1); // safety: test + assert_eq!(memory.failed_actions(), 1); // safety: test } #[test] fn test_memory_last_action() { let mut memory = Memory::new(Uuid::new_v4()); - assert!(memory.last_action().is_none()); + assert!(memory.last_action().is_none()); // safety: test let a1 = memory .create_action("first", serde_json::json!({})) @@ -478,8 +487,8 @@ mod tests { .fail("nope", Duration::ZERO); memory.record_action(a2); - let last = memory.last_action().unwrap(); - assert_eq!(last.tool_name, "second"); + let last = memory.last_action().unwrap(); // safety: test + assert_eq!(last.tool_name, "second"); // safety: test } #[test] @@ -499,9 +508,9 @@ mod tests { ); memory.record_action(a); - assert_eq!(memory.actions_by_tool("shell").len(), 3); - assert_eq!(memory.actions_by_tool("http").len(), 1); - assert_eq!(memory.actions_by_tool("nonexistent").len(), 0); + assert_eq!(memory.actions_by_tool("shell").len(), 3); // safety: test + assert_eq!(memory.actions_by_tool("http").len(), 1); // safety: test + assert_eq!(memory.actions_by_tool("nonexistent").len(), 0); // safety: test } #[test] @@ -509,25 +518,25 @@ mod tests { let mut memory = Memory::new(Uuid::new_v4()); let a0 = memory.create_action("t", serde_json::json!({})); - assert_eq!(a0.sequence, 0); + assert_eq!(a0.sequence, 0); // safety: test let a1 = memory.create_action("t", serde_json::json!({})); - assert_eq!(a1.sequence, 1); + assert_eq!(a1.sequence, 1); // safety: test let a2 = memory.create_action("t", serde_json::json!({})); - assert_eq!(a2.sequence, 2); + assert_eq!(a2.sequence, 2); // safety: test } #[test] fn test_memory_add_message_delegates_to_conversation() { let mut memory = Memory::new(Uuid::new_v4()); - assert!(memory.conversation.is_empty()); + assert!(memory.conversation.is_empty()); // safety: test memory.add_message(ChatMessage::user("hello")); memory.add_message(ChatMessage::assistant("hi")); - assert_eq!(memory.conversation.len(), 2); - assert_eq!(memory.conversation.messages()[0].content, "hello"); + assert_eq!(memory.conversation.len(), 2); // safety: test + assert_eq!(memory.conversation.messages()[0].content, "hello"); // safety: test } #[test] @@ -540,7 +549,7 @@ mod tests { .succeed(None, serde_json::json!({}), Duration::ZERO); memory.record_action(a); - assert_eq!(memory.total_cost(), Decimal::ZERO); + assert_eq!(memory.total_cost(), Decimal::ZERO); // safety: test } #[test] @@ -560,6 +569,6 @@ mod tests { memory.record_action(a2); // Both successful and failed actions contribute to total duration - assert_eq!(memory.total_duration(), Duration::from_millis(300)); + assert_eq!(memory.total_duration(), Duration::from_millis(300)); // safety: test } } diff --git a/src/context/mod.rs b/src/context/mod.rs index a7dd61de..4b482038 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -6,10 +6,12 @@ //! - State machine //! - Resource tracking +pub mod fallback; mod manager; mod memory; mod state; +pub use fallback::FallbackDeliverable; pub use manager::ContextManager; pub use memory::{ActionRecord, ConversationMemory, Memory}; pub use state::{JobContext, JobState, StateTransition, TokenBudgetExceeded}; diff --git a/src/orchestrator/api.rs b/src/orchestrator/api.rs index b46aa8c6..8d77c581 100644 --- a/src/orchestrator/api.rs +++ b/src/orchestrator/api.rs @@ -333,6 +333,12 @@ async fn job_event_handler( .get("session_id") .and_then(|v| v.as_str()) .map(|s| s.to_string()), + // NOTE: `fallback_deliverable` is currently always None in SSE events. + // In-memory jobs store fallback data in JobContext.metadata (accessed via job_status tool). + // Sandbox containers don't yet emit fallback data in their event payloads. + // This field is forward-compatible infrastructure for when container workers + // gain context/memory tracking capabilities. + fallback_deliverable: payload.data.get("fallback_deliverable").cloned(), }, _ => SseEvent::JobStatus { job_id: job_id_str, diff --git a/src/tools/builtin/job.rs b/src/tools/builtin/job.rs index 9346d14a..ea7e5305 100644 --- a/src/tools/builtin/job.rs +++ b/src/tools/builtin/job.rs @@ -1005,7 +1005,8 @@ impl Tool for JobStatusTool { "created_at": job_ctx.created_at.to_rfc3339(), "started_at": job_ctx.started_at.map(|t| t.to_rfc3339()), "completed_at": job_ctx.completed_at.map(|t| t.to_rfc3339()), - "actual_cost": job_ctx.actual_cost.to_string() + "actual_cost": job_ctx.actual_cost.to_string(), + "fallback_deliverable": job_ctx.metadata.get("fallback_deliverable"), }); Ok(ToolOutput::success(result, start.elapsed())) } @@ -1384,7 +1385,7 @@ mod tests { let tool = CreateJobTool::new(manager.clone()); // Without sandbox deps, it should use the local path - assert!(!tool.sandbox_enabled()); + assert!(!tool.sandbox_enabled()); // safety: test let params = serde_json::json!({ "title": "Test Job", @@ -1392,12 +1393,13 @@ mod tests { }); let ctx = JobContext::default(); - let result = tool.execute(params, &ctx).await.unwrap(); + let result = tool.execute(params, &ctx).await.unwrap(); // safety: test - let job_id = result.result.get("job_id").unwrap().as_str().unwrap(); - assert!(!job_id.is_empty()); + let job_id = result.result.get("job_id").unwrap().as_str().unwrap(); // safety: test + assert!(!job_id.is_empty()); // safety: test assert_eq!( - result.result.get("status").unwrap().as_str().unwrap(), + /* safety: test */ + result.result.get("status").unwrap().as_str().unwrap(), // safety: test "pending" ); } @@ -1409,11 +1411,11 @@ mod tests { // Without sandbox let tool = CreateJobTool::new(Arc::clone(&manager)); let schema = tool.parameters_schema(); - let props = schema.get("properties").unwrap().as_object().unwrap(); - assert!(props.contains_key("title")); - assert!(props.contains_key("description")); - assert!(!props.contains_key("wait")); - assert!(!props.contains_key("mode")); + let props = schema.get("properties").unwrap().as_object().unwrap(); // safety: test + assert!(props.contains_key("title")); // safety: test + assert!(props.contains_key("description")); // safety: test + assert!(!props.contains_key("wait")); // safety: test + assert!(!props.contains_key("mode")); // safety: test } #[test] @@ -1422,7 +1424,7 @@ mod tests { // Without sandbox: default timeout let tool = CreateJobTool::new(Arc::clone(&manager)); - assert_eq!(tool.execution_timeout(), Duration::from_secs(30)); + assert_eq!(tool.execution_timeout(), Duration::from_secs(30)); // safety: test } #[tokio::test] @@ -1455,23 +1457,23 @@ mod tests { let manager = Arc::new(ContextManager::new(5)); // Create some jobs - manager.create_job("Job 1", "Desc 1").await.unwrap(); - manager.create_job("Job 2", "Desc 2").await.unwrap(); + manager.create_job("Job 1", "Desc 1").await.unwrap(); // safety: test + manager.create_job("Job 2", "Desc 2").await.unwrap(); // safety: test let tool = ListJobsTool::new(manager); let params = serde_json::json!({}); let ctx = JobContext::default(); - let result = tool.execute(params, &ctx).await.unwrap(); + let result = tool.execute(params, &ctx).await.unwrap(); // safety: test - let jobs = result.result.get("jobs").unwrap().as_array().unwrap(); - assert_eq!(jobs.len(), 2); + let jobs = result.result.get("jobs").unwrap().as_array().unwrap(); // safety: test + assert_eq!(jobs.len(), 2); // safety: test } #[tokio::test] async fn test_job_status_tool() { let manager = Arc::new(ContextManager::new(5)); - let job_id = manager.create_job("Test Job", "Description").await.unwrap(); + let job_id = manager.create_job("Test Job", "Description").await.unwrap(); // safety: test let tool = JobStatusTool::new(manager); @@ -1479,10 +1481,11 @@ mod tests { "job_id": job_id.to_string() }); let ctx = JobContext::default(); - let result = tool.execute(params, &ctx).await.unwrap(); + let result = tool.execute(params, &ctx).await.unwrap(); // safety: test assert_eq!( - result.result.get("title").unwrap().as_str().unwrap(), + /* safety: test */ + result.result.get("title").unwrap().as_str().unwrap(), // safety: test "Test Job" ); } @@ -1496,8 +1499,9 @@ mod tests { let missing_title = tool .execute(serde_json::json!({ "description": "A test job" }), &ctx) .await; - assert!(missing_title.is_err()); + assert!(missing_title.is_err()); // safety: test assert!( + /* safety: test */ missing_title .unwrap_err() .to_string() @@ -1507,8 +1511,9 @@ mod tests { let missing_description = tool .execute(serde_json::json!({ "title": "Test Job" }), &ctx) .await; - assert!(missing_description.is_err()); + assert!(missing_description.is_err()); // safety: test assert!( + /* safety: test */ missing_description .unwrap_err() .to_string() @@ -1522,19 +1527,19 @@ mod tests { let pending_id = manager .create_job_for_user("default", "Pending Job", "Todo") .await - .unwrap(); + .unwrap(); // safety: test let completed_id = manager .create_job_for_user("default", "Completed Job", "Done") .await - .unwrap(); + .unwrap(); // safety: test let failed_id = manager .create_job_for_user("default", "Failed Job", "Oops") .await - .unwrap(); + .unwrap(); // safety: test manager .create_job_for_user("other-user", "Other User Job", "Ignore") .await - .unwrap(); + .unwrap(); // safety: test manager .update_context(completed_id, |ctx| { @@ -1542,41 +1547,44 @@ mod tests { ctx.transition_to(JobState::Completed, Some("done".to_string())) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test manager .update_context(failed_id, |ctx| { ctx.transition_to(JobState::InProgress, None)?; ctx.transition_to(JobState::Failed, Some("boom".to_string())) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test let tool = ListJobsTool::new(Arc::clone(&manager)); let ctx = JobContext::default(); - let result = tool.execute(serde_json::json!({}), &ctx).await.unwrap(); + let result = tool.execute(serde_json::json!({}), &ctx).await.unwrap(); // safety: test - let jobs = result.result.get("jobs").unwrap().as_array().unwrap(); - assert_eq!(jobs.len(), 3); + let jobs = result.result.get("jobs").unwrap().as_array().unwrap(); // safety: test + assert_eq!(jobs.len(), 3); // safety: test assert!(jobs.iter().any(|job| { + // safety: test job.get("job_id").and_then(|v| v.as_str()) == Some(&pending_id.to_string()) && job.get("status").and_then(|v| v.as_str()) == Some("Pending") })); assert!(jobs.iter().any(|job| { + // safety: test job.get("job_id").and_then(|v| v.as_str()) == Some(&completed_id.to_string()) && job.get("status").and_then(|v| v.as_str()) == Some("Completed") })); assert!(jobs.iter().any(|job| { + // safety: test job.get("job_id").and_then(|v| v.as_str()) == Some(&failed_id.to_string()) && job.get("status").and_then(|v| v.as_str()) == Some("Failed") })); - let summary = result.result.get("summary").unwrap(); - assert_eq!(summary.get("total").and_then(|v| v.as_u64()), Some(3)); - assert_eq!(summary.get("pending").and_then(|v| v.as_u64()), Some(1)); - assert_eq!(summary.get("completed").and_then(|v| v.as_u64()), Some(1)); - assert_eq!(summary.get("failed").and_then(|v| v.as_u64()), Some(1)); + let summary = result.result.get("summary").unwrap(); // safety: test + assert_eq!(summary.get("total").and_then(|v| v.as_u64()), Some(3)); // safety: test + assert_eq!(summary.get("pending").and_then(|v| v.as_u64()), Some(1)); // safety: test + assert_eq!(summary.get("completed").and_then(|v| v.as_u64()), Some(1)); // safety: test + assert_eq!(summary.get("failed").and_then(|v| v.as_u64()), Some(1)); // safety: test } #[tokio::test] @@ -1585,29 +1593,30 @@ mod tests { let job_id = manager .create_job_for_user("default", "Transition Job", "Track me") .await - .unwrap(); + .unwrap(); // safety: test manager .update_context(job_id, |ctx| { ctx.transition_to(JobState::InProgress, Some("started".to_string()))?; ctx.transition_to(JobState::Completed, Some("finished".to_string())) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test let tool = JobStatusTool::new(Arc::clone(&manager)); let ctx = JobContext::default(); let result = tool .execute(serde_json::json!({ "job_id": job_id.to_string() }), &ctx) .await - .unwrap(); + .unwrap(); // safety: test assert_eq!( + /* safety: test */ result.result.get("status").and_then(|v| v.as_str()), Some("Completed") ); - assert!(result.result.get("started_at").unwrap().is_string()); - assert!(result.result.get("completed_at").unwrap().is_string()); + assert!(result.result.get("started_at").unwrap().is_string()); // safety: test + assert!(result.result.get("completed_at").unwrap().is_string()); // safety: test } #[tokio::test] @@ -1616,26 +1625,27 @@ mod tests { let job_id = manager .create_job_for_user("default", "Running Job", "In progress") .await - .unwrap(); + .unwrap(); // safety: test manager .update_context(job_id, |ctx| ctx.transition_to(JobState::InProgress, None)) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test let tool = CancelJobTool::new(Arc::clone(&manager)); let ctx = JobContext::default(); let result = tool .execute(serde_json::json!({ "job_id": job_id.to_string() }), &ctx) .await - .unwrap(); + .unwrap(); // safety: test assert_eq!( + /* safety: test */ result.result.get("status").and_then(|v| v.as_str()), Some("cancelled") ); - let updated = manager.get_context(job_id).await.unwrap(); - assert_eq!(updated.state, JobState::Cancelled); + let updated = manager.get_context(job_id).await.unwrap(); // safety: test + assert_eq!(updated.state, JobState::Cancelled); // safety: test } #[tokio::test] @@ -1644,39 +1654,81 @@ mod tests { let job_id = manager .create_job_for_user("default", "Completed Job", "Already done") .await - .unwrap(); + .unwrap(); // safety: test manager .update_context(job_id, |ctx| { ctx.transition_to(JobState::InProgress, None)?; ctx.transition_to(JobState::Completed, Some("done".to_string())) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test let tool = CancelJobTool::new(Arc::clone(&manager)); let ctx = JobContext::default(); let result = tool .execute(serde_json::json!({ "job_id": job_id.to_string() }), &ctx) .await - .unwrap(); + .unwrap(); // safety: test - let error = result.result.get("error").and_then(|v| v.as_str()).unwrap(); - assert!(error.contains("Cannot cancel job")); - assert!(error.contains("completed")); + let error = result.result.get("error").and_then(|v| v.as_str()).unwrap(); // safety: test + assert!(error.contains("Cannot cancel job")); // safety: test + assert!(error.contains("completed")); // safety: test + } + + #[tokio::test] + async fn test_job_status_includes_fallback_deliverable() { + let manager = Arc::new(ContextManager::new(5)); + let job_id = manager + .create_job_for_user("default", "Failing Job", "Will fail") + .await + .unwrap(); // safety: test + + // Inject a real FallbackDeliverable into the job metadata. + let fallback = serde_json::json!({ + "partial": true, + "failure_reason": "max iterations", + "last_action": null, + "action_stats": { "total": 5, "successful": 3, "failed": 2 }, + "tokens_used": 1000, + "cost": "0.05", + "elapsed_secs": 12.5, + "repair_attempts": 1, + }); + manager + .update_context(job_id, |ctx| { + ctx.metadata = serde_json::json!({ "fallback_deliverable": fallback.clone() }); + Ok::<(), String>(()) + }) + .await + .unwrap() // safety: test + .unwrap(); // safety: test + + let tool = JobStatusTool::new(manager); + let params = serde_json::json!({ "job_id": job_id.to_string() }); + let ctx = JobContext::default(); + let result = tool.execute(params, &ctx).await.unwrap(); // safety: test + + let fb = result.result.get("fallback_deliverable").unwrap(); // safety: test + assert_eq!(fb.get("partial").unwrap(), true); // safety: test + assert_eq!(fb.get("failure_reason").unwrap(), "max iterations"); // safety: test + let stats = fb.get("action_stats").unwrap(); // safety: test + assert_eq!(stats.get("total").unwrap(), 5); // safety: test + assert_eq!(stats.get("successful").unwrap(), 3); // safety: test + assert_eq!(stats.get("failed").unwrap(), 2); // safety: test } #[test] fn test_resolve_project_dir_auto() { let project_id = Uuid::new_v4(); - let (dir, browse_id) = resolve_project_dir(None, project_id).unwrap(); - assert!(dir.exists()); - assert!(dir.ends_with(project_id.to_string())); - assert_eq!(browse_id, project_id.to_string()); + let (dir, browse_id) = resolve_project_dir(None, project_id).unwrap(); // safety: test + assert!(dir.exists()); // safety: test + assert!(dir.ends_with(project_id.to_string())); // safety: test + assert_eq!(browse_id, project_id.to_string()); // safety: test // Must be under the projects base - let base = projects_base().canonicalize().unwrap(); - assert!(dir.starts_with(&base)); + let base = projects_base().canonicalize().unwrap(); // safety: test + assert!(dir.starts_with(&base)); // safety: test let _ = std::fs::remove_dir_all(&dir); } @@ -1684,33 +1736,34 @@ mod tests { #[test] fn test_resolve_project_dir_explicit_under_base() { let base = projects_base(); - std::fs::create_dir_all(&base).unwrap(); + std::fs::create_dir_all(&base).unwrap(); // safety: test let explicit = base.join("test_explicit_project"); // Explicit paths must already exist (no auto-create). - std::fs::create_dir_all(&explicit).unwrap(); + std::fs::create_dir_all(&explicit).unwrap(); // safety: test let project_id = Uuid::new_v4(); - let (dir, browse_id) = resolve_project_dir(Some(explicit.clone()), project_id).unwrap(); - assert!(dir.exists()); - assert_eq!(browse_id, "test_explicit_project"); + let (dir, browse_id) = resolve_project_dir(Some(explicit.clone()), project_id).unwrap(); // safety: test + assert!(dir.exists()); // safety: test + assert_eq!(browse_id, "test_explicit_project"); // safety: test - let canonical_base = base.canonicalize().unwrap(); - assert!(dir.starts_with(&canonical_base)); + let canonical_base = base.canonicalize().unwrap(); // safety: test + assert!(dir.starts_with(&canonical_base)); // safety: test let _ = std::fs::remove_dir_all(&explicit); } #[test] fn test_resolve_project_dir_rejects_outside_base() { - let tmp = tempfile::tempdir().unwrap(); + let tmp = tempfile::tempdir().unwrap(); // safety: test let escape_attempt = tmp.path().join("evil_project"); // Don't create it: explicit paths that don't exist are rejected // before the prefix check even runs. let result = resolve_project_dir(Some(escape_attempt), Uuid::new_v4()); - assert!(result.is_err()); + assert!(result.is_err()); // safety: test let err = result.unwrap_err().to_string(); assert!( + /* safety: test */ err.contains("does not exist"), "expected 'does not exist' error, got: {}", err @@ -1720,13 +1773,14 @@ mod tests { #[test] fn test_resolve_project_dir_rejects_outside_base_existing() { // A directory that exists but is outside the projects base. - let tmp = tempfile::tempdir().unwrap(); + let tmp = tempfile::tempdir().unwrap(); // safety: test let outside = tmp.path().to_path_buf(); let result = resolve_project_dir(Some(outside), Uuid::new_v4()); - assert!(result.is_err()); + assert!(result.is_err()); // safety: test let err = result.unwrap_err().to_string(); assert!( + /* safety: test */ err.contains("must be under"), "expected 'must be under' error, got: {}", err @@ -1740,7 +1794,7 @@ mod tests { let traversal = base.join("legit").join("..").join("..").join(".ssh"); let result = resolve_project_dir(Some(traversal), Uuid::new_v4()); - assert!(result.is_err(), "traversal path should be rejected"); + assert!(result.is_err(), "traversal path should be rejected"); // safety: test // Traversal path that actually resolves gets the prefix check. // `base/../` resolves to the parent of projects base, which is outside. @@ -1748,7 +1802,7 @@ mod tests { std::fs::create_dir_all(&base_parent).ok(); if base_parent.exists() { let result = resolve_project_dir(Some(base_parent.clone()), Uuid::new_v4()); - assert!(result.is_err(), "path outside base should be rejected"); + assert!(result.is_err(), "path outside base should be rejected"); // safety: test let _ = std::fs::remove_dir_all(&base_parent); } } @@ -1762,8 +1816,9 @@ mod tests { )); let tool = CreateJobTool::new(manager).with_sandbox(jm, None); let schema = tool.parameters_schema(); - let props = schema.get("properties").unwrap().as_object().unwrap(); + let props = schema.get("properties").unwrap().as_object().unwrap(); // safety: test assert!( + /* safety: test */ props.contains_key("project_dir"), "sandbox schema must expose project_dir" ); @@ -1778,8 +1833,9 @@ mod tests { )); let tool = CreateJobTool::new(manager).with_sandbox(jm, None); let schema = tool.parameters_schema(); - let props = schema.get("properties").unwrap().as_object().unwrap(); + let props = schema.get("properties").unwrap().as_object().unwrap(); // safety: test assert!( + /* safety: test */ props.contains_key("credentials"), "sandbox schema must expose credentials" ); @@ -1792,13 +1848,13 @@ mod tests { // No credentials parameter let params = serde_json::json!({"title": "t", "description": "d"}); - let grants = tool.parse_credentials(¶ms, "user1").await.unwrap(); - assert!(grants.is_empty()); + let grants = tool.parse_credentials(¶ms, "user1").await.unwrap(); // safety: test + assert!(grants.is_empty()); // safety: test // Empty credentials object let params = serde_json::json!({"credentials": {}}); - let grants = tool.parse_credentials(¶ms, "user1").await.unwrap(); - assert!(grants.is_empty()); + let grants = tool.parse_credentials(¶ms, "user1").await.unwrap(); // safety: test + assert!(grants.is_empty()); // safety: test } #[tokio::test] @@ -1808,9 +1864,10 @@ mod tests { let params = serde_json::json!({"credentials": {"my_secret": "MY_SECRET"}}); let result = tool.parse_credentials(¶ms, "user1").await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test let err = result.unwrap_err().to_string(); assert!( + /* safety: test */ err.contains("no secrets store"), "expected 'no secrets store' error, got: {}", err @@ -1828,9 +1885,10 @@ mod tests { let params = serde_json::json!({"credentials": {"nonexistent_secret": "SOME_VAR"}}); let result = tool.parse_credentials(¶ms, "user1").await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test let err = result.unwrap_err().to_string(); assert!( + /* safety: test */ err.contains("not found"), "expected 'not found' error, got: {}", err @@ -1852,17 +1910,17 @@ mod tests { CreateSecretParams::new("github_token", TEST_GITHUB_TOKEN), ) .await - .unwrap(); + .unwrap(); // safety: test let tool = CreateJobTool::new(manager).with_secrets(Arc::clone(&secrets)); let params = serde_json::json!({ "credentials": {"github_token": "GITHUB_TOKEN"} }); - let grants = tool.parse_credentials(¶ms, "user1").await.unwrap(); - assert_eq!(grants.len(), 1); - assert_eq!(grants[0].secret_name, "github_token"); - assert_eq!(grants[0].env_var, "GITHUB_TOKEN"); + let grants = tool.parse_credentials(¶ms, "user1").await.unwrap(); // safety: test + assert_eq!(grants.len(), 1); // safety: test + assert_eq!(grants[0].secret_name, "github_token"); // safety: test + assert_eq!(grants[0].env_var, "GITHUB_TOKEN"); // safety: test } fn test_prompt_tool(queue: PromptQueue) -> JobPromptTool { @@ -1876,7 +1934,7 @@ mod tests { let job_id = cm .create_job_for_user("default", "Test Job", "desc") .await - .unwrap(); + .unwrap(); // safety: test let queue: PromptQueue = Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())); @@ -1889,18 +1947,19 @@ mod tests { }); let ctx = JobContext::default(); - let result = tool.execute(params, &ctx).await.unwrap(); + let result = tool.execute(params, &ctx).await.unwrap(); // safety: test assert_eq!( - result.result.get("status").unwrap().as_str().unwrap(), + /* safety: test */ + result.result.get("status").unwrap().as_str().unwrap(), // safety: test "queued" ); let q = queue.lock().await; - let prompts = q.get(&job_id).unwrap(); - assert_eq!(prompts.len(), 1); - assert_eq!(prompts[0].content, "What's the status?"); - assert!(!prompts[0].done); + let prompts = q.get(&job_id).unwrap(); // safety: test + assert_eq!(prompts.len(), 1); // safety: test + assert_eq!(prompts[0].content, "What's the status?"); // safety: test + assert!(!prompts[0].done); // safety: test } #[tokio::test] @@ -1910,6 +1969,7 @@ mod tests { Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())); let tool = test_prompt_tool(queue); assert_eq!( + /* safety: test */ tool.requires_approval(&serde_json::json!({})), ApprovalRequirement::UnlessAutoApproved ); @@ -1928,7 +1988,7 @@ mod tests { let ctx = JobContext::default(); let result = tool.execute(params, &ctx).await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test } #[tokio::test] @@ -1943,7 +2003,7 @@ mod tests { let ctx = JobContext::default(); let result = tool.execute(params, &ctx).await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test } #[tokio::test] @@ -1958,7 +2018,7 @@ mod tests { let job_id = cm .create_job_for_user("owner-user", "Secret Job", "classified") .await - .unwrap(); + .unwrap(); // safety: test // We need a Store to construct the tool, but creating one requires // a database URL. Instead, test the ownership logic directly: @@ -1968,9 +2028,9 @@ mod tests { ..Default::default() }; - let job_ctx = cm.get_context(job_id).await.unwrap(); - assert_ne!(job_ctx.user_id, attacker_ctx.user_id); - assert_eq!(job_ctx.user_id, "owner-user"); + let job_ctx = cm.get_context(job_id).await.unwrap(); // safety: test + assert_ne!(job_ctx.user_id, attacker_ctx.user_id); // safety: test + assert_eq!(job_ctx.user_id, "owner-user"); // safety: test } #[test] @@ -1991,12 +2051,12 @@ mod tests { "required": ["job_id"] }); - let props = schema.get("properties").unwrap().as_object().unwrap(); - assert!(props.contains_key("job_id")); - assert!(props.contains_key("limit")); - let required = schema.get("required").unwrap().as_array().unwrap(); - assert_eq!(required.len(), 1); - assert_eq!(required[0].as_str().unwrap(), "job_id"); + let props = schema.get("properties").unwrap().as_object().unwrap(); // safety: test + assert!(props.contains_key("job_id")); // safety: test + assert!(props.contains_key("limit")); // safety: test + let required = schema.get("required").unwrap().as_array().unwrap(); // safety: test + assert_eq!(required.len(), 1); // safety: test + assert_eq!(required[0].as_str().unwrap(), "job_id"); // safety: test } #[tokio::test] @@ -2005,7 +2065,7 @@ mod tests { let job_id = cm .create_job_for_user("owner-user", "Test Job", "desc") .await - .unwrap(); + .unwrap(); // safety: test let queue: PromptQueue = Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())); @@ -2023,9 +2083,10 @@ mod tests { }; let result = tool.execute(params, &ctx).await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test let err = result.unwrap_err().to_string(); assert!( + /* safety: test */ err.contains("does not belong to current user"), "expected ownership error, got: {}", err @@ -2035,33 +2096,34 @@ mod tests { #[tokio::test] async fn test_resolve_job_id_full_uuid() { let cm = ContextManager::new(5); - let job_id = cm.create_job("Test", "Desc").await.unwrap(); + let job_id = cm.create_job("Test", "Desc").await.unwrap(); // safety: test - let resolved = resolve_job_id(&job_id.to_string(), &cm).await.unwrap(); - assert_eq!(resolved, job_id); + let resolved = resolve_job_id(&job_id.to_string(), &cm).await.unwrap(); // safety: test + assert_eq!(resolved, job_id); // safety: test } #[tokio::test] async fn test_resolve_job_id_short_prefix() { let cm = ContextManager::new(5); - let job_id = cm.create_job("Test", "Desc").await.unwrap(); + let job_id = cm.create_job("Test", "Desc").await.unwrap(); // safety: test // Use first 8 hex chars (without dashes) let hex = job_id.to_string().replace('-', ""); let prefix = &hex[..8]; - let resolved = resolve_job_id(prefix, &cm).await.unwrap(); - assert_eq!(resolved, job_id); + let resolved = resolve_job_id(prefix, &cm).await.unwrap(); // safety: test + assert_eq!(resolved, job_id); // safety: test } #[tokio::test] async fn test_resolve_job_id_no_match() { let cm = ContextManager::new(5); - cm.create_job("Test", "Desc").await.unwrap(); + cm.create_job("Test", "Desc").await.unwrap(); // safety: test let result = resolve_job_id("00000000", &cm).await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test let err = result.unwrap_err().to_string(); assert!( + /* safety: test */ err.contains("no job found"), "expected 'no job found', got: {}", err @@ -2072,6 +2134,6 @@ mod tests { async fn test_resolve_job_id_invalid_input() { let cm = ContextManager::new(5); let result = resolve_job_id("not-hex-at-all!", &cm).await; - assert!(result.is_err()); + assert!(result.is_err()); // safety: test } } diff --git a/src/worker/job.rs b/src/worker/job.rs index 0f0e969e..87b9cfeb 100644 --- a/src/worker/job.rs +++ b/src/worker/job.rs @@ -196,6 +196,7 @@ impl Worker { .get("session_id") .and_then(|v| v.as_str()) .map(|s| s.to_string()), + fallback_deliverable: data.get("fallback_deliverable").cloned(), }), _ => None, }; @@ -960,9 +961,14 @@ Report when the job is complete or if you encounter issues you cannot resolve."# } async fn mark_failed(&self, reason: &str) -> Result<(), Error> { + // Build fallback deliverable from memory before transitioning. + let fallback = self.build_fallback(reason).await; + self.context_manager() .update_context(self.job_id, |ctx| { - ctx.transition_to(JobState::Failed, Some(reason.to_string())) + ctx.transition_to(JobState::Failed, Some(reason.to_string()))?; + store_fallback_in_metadata(ctx, fallback.as_ref()); + Ok(()) }) .await? .map_err(|s| crate::error::JobError::ContextError { @@ -983,8 +989,15 @@ Report when the job is complete or if you encounter issues you cannot resolve."# } async fn mark_stuck(&self, reason: &str) -> Result<(), Error> { + // Build fallback deliverable from memory before transitioning. + let fallback = self.build_fallback(reason).await; + self.context_manager() - .update_context(self.job_id, |ctx| ctx.mark_stuck(reason)) + .update_context(self.job_id, |ctx| { + ctx.mark_stuck(reason)?; + store_fallback_in_metadata(ctx, fallback.as_ref()); + Ok(()) + }) .await? .map_err(|s| crate::error::JobError::ContextError { id: self.job_id, @@ -1002,6 +1015,57 @@ Report when the job is complete or if you encounter issues you cannot resolve."# self.persist_status(JobState::Stuck, Some(reason.to_string())); Ok(()) } + + /// Build a [`FallbackDeliverable`] from the current job context and memory. + async fn build_fallback(&self, reason: &str) -> Option { + let memory = match self.context_manager().get_memory(self.job_id).await { + Ok(memory) => memory, + Err(e) => { + tracing::warn!( + job_id = %self.job_id, + "Failed to load memory while building fallback deliverable: {e}" + ); + return None; + } + }; + let ctx = match self.context_manager().get_context(self.job_id).await { + Ok(ctx) => ctx, + Err(e) => { + tracing::warn!( + job_id = %self.job_id, + "Failed to load context while building fallback deliverable: {e}" + ); + return None; + } + }; + Some(crate::context::FallbackDeliverable::build( + &ctx, &memory, reason, + )) + } +} + +/// Store a fallback deliverable in the job context's metadata. +fn store_fallback_in_metadata( + ctx: &mut crate::context::JobContext, + fallback: Option<&crate::context::FallbackDeliverable>, +) { + let Some(fb) = fallback else { + return; + }; + match serde_json::to_value(fb) { + Ok(val) => { + if !ctx.metadata.is_object() { + ctx.metadata = serde_json::json!({}); + } + ctx.metadata["fallback_deliverable"] = val; + } + Err(e) => { + tracing::warn!( + "Failed to serialize fallback deliverable for job {}: {e}", + ctx.job_id + ); + } + } } /// Job delegate: implements `LoopDelegate` for the background job context. @@ -1440,7 +1504,7 @@ mod tests { } let cm = Arc::new(crate::context::ContextManager::new(5)); - let job_id = cm.create_job("test", "test job").await.unwrap(); + let job_id = cm.create_job("test", "test job").await.unwrap(); // safety: test let deps = WorkerDeps { context_manager: cm, @@ -1472,8 +1536,9 @@ mod tests { tool_call_id: "call_abc123".to_string(), }; - assert_eq!(selection.tool_call_id, "call_abc123"); + assert_eq!(selection.tool_call_id, "call_abc123"); // safety: test assert_ne!( + /* safety: test */ selection.tool_call_id, "tool_call_id", "tool_call_id must not be the hardcoded placeholder string" ); @@ -1509,11 +1574,12 @@ mod tests { let results = worker.execute_tools_parallel(&selections).await; let elapsed = start.elapsed(); - assert_eq!(results.len(), 3); + assert_eq!(results.len(), 3); // safety: test for r in &results { - assert!(r.result.is_ok(), "Tool should succeed"); + assert!(r.result.is_ok(), "Tool should succeed"); // safety: test } assert!( + /* safety: test */ elapsed < Duration::from_millis(800), "Parallel execution took {:?}, expected < 800ms (sequential would be ~600ms)", elapsed @@ -1565,9 +1631,9 @@ mod tests { let results = worker.execute_tools_parallel(&selections).await; - assert!(results[0].result.as_ref().unwrap().contains("done_tool_a")); - assert!(results[1].result.as_ref().unwrap().contains("done_tool_b")); - assert!(results[2].result.as_ref().unwrap().contains("done_tool_c")); + assert!(results[0].result.as_ref().unwrap().contains("done_tool_a")); // safety: test + assert!(results[1].result.as_ref().unwrap().contains("done_tool_b")); // safety: test + assert!(results[2].result.as_ref().unwrap().contains("done_tool_c")); // safety: test } #[tokio::test] @@ -1583,8 +1649,9 @@ mod tests { }]; let results = worker.execute_tools_parallel(&selections).await; - assert_eq!(results.len(), 1); + assert_eq!(results.len(), 1); // safety: test assert!( + /* safety: test */ results[0].result.is_err(), "Missing tool should produce an error, not a panic" ); @@ -1600,23 +1667,24 @@ mod tests { ctx.transition_to(JobState::InProgress, None) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test - worker.mark_completed().await.unwrap(); + worker.mark_completed().await.unwrap(); // safety: test let ctx = worker .context_manager() .get_context(worker.job_id) .await - .unwrap(); - assert_eq!(ctx.state, JobState::Completed); + .unwrap(); // safety: test + assert_eq!(ctx.state, JobState::Completed); // safety: test // Second mark_completed should succeed (idempotent) rather than // erroring, matching the fix for the execution_loop / worker wrapper // race condition. let result = worker.mark_completed().await; assert!( + /* safety: test */ result.is_ok(), "Completed -> Completed transition should be idempotent" ); @@ -1641,7 +1709,7 @@ mod tests { } let cm = Arc::new(crate::context::ContextManager::new(5)); - let job_id = cm.create_job("test", "test job").await.unwrap(); + let job_id = cm.create_job("test", "test job").await.unwrap(); // safety: test let deps = WorkerDeps { context_manager: cm, @@ -1740,6 +1808,7 @@ mod tests { .execute_tool("needs_approval", &serde_json::json!({})) .await; assert!( + /* safety: test */ result.is_err(), "Should be blocked without approval context" ); @@ -1752,7 +1821,7 @@ mod tests { let result = worker_allowed .execute_tool("needs_approval", &serde_json::json!({})) .await; - assert!(result.is_ok(), "Should be allowed with autonomous context"); + assert!(result.is_ok(), "Should be allowed with autonomous context"); // safety: test } #[tokio::test] @@ -1766,6 +1835,7 @@ mod tests { .execute_tool("always_approval", &serde_json::json!({})) .await; assert!( + /* safety: test */ result.is_err(), "Always tool should be blocked without permission" ); @@ -1781,6 +1851,7 @@ mod tests { .execute_tool("always_approval", &serde_json::json!({})) .await; assert!( + /* safety: test */ result.is_ok(), "Always tool should be allowed with permission" ); @@ -1797,8 +1868,8 @@ mod tests { ctx.transition_to(JobState::InProgress, None) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test // Set a token budget worker @@ -1807,16 +1878,17 @@ mod tests { ctx.max_tokens = 100; }) .await - .unwrap(); + .unwrap(); // safety: test // Simulate adding tokens that exceed the budget let budget_result = worker .context_manager() .update_context(worker.job_id, |ctx| ctx.add_tokens(200)) .await - .unwrap(); + .unwrap(); // safety: test assert!( + /* safety: test */ budget_result.is_err(), "Should return error when token budget exceeded" ); @@ -1825,13 +1897,13 @@ mod tests { worker .mark_failed(&budget_result.unwrap_err().to_string()) .await - .unwrap(); + .unwrap(); // safety: test let ctx = worker .context_manager() .get_context(worker.job_id) .await - .unwrap(); - assert_eq!(ctx.state, JobState::Failed); + .unwrap(); // safety: test + assert_eq!(ctx.state, JobState::Failed); // safety: test } #[tokio::test] @@ -1845,21 +1917,22 @@ mod tests { ctx.transition_to(JobState::InProgress, None) }) .await - .unwrap() - .unwrap(); + .unwrap() // safety: test + .unwrap(); // safety: test // Simulate what the execution loop does when max_iterations is exceeded worker .mark_failed("Maximum iterations exceeded: job hit the iteration cap") .await - .unwrap(); + .unwrap(); // safety: test let ctx = worker .context_manager() .get_context(worker.job_id) .await - .unwrap(); + .unwrap(); // safety: test assert_eq!( + /* safety: test */ ctx.state, JobState::Failed, "Iteration cap should transition to Failed, not Stuck" @@ -1989,4 +2062,52 @@ mod tests { "Should skip empty first reasoning and return the first non-empty one" ); } + + #[test] + fn test_store_fallback_in_metadata_roundtrip() { + use crate::context::FallbackDeliverable; + + let mut ctx = JobContext::new("Test", "fallback roundtrip"); + let memory = crate::context::Memory::new(ctx.job_id); + let fb = FallbackDeliverable::build(&ctx, &memory, "test failure"); + + // Store into metadata + store_fallback_in_metadata(&mut ctx, Some(&fb)); + + // Verify it's stored and can be deserialized back + let stored = ctx.metadata.get("fallback_deliverable"); + assert!(stored.is_some(), "fallback missing from metadata"); // safety: test + + let recovered: FallbackDeliverable = + serde_json::from_value(stored.unwrap().clone()).expect("deserialize fallback"); // safety: test + assert_eq!(recovered.failure_reason, "test failure"); // safety: test + assert!(!recovered.partial); // safety: test + } + + #[test] + fn test_store_fallback_handles_non_object_metadata() { + use crate::context::FallbackDeliverable; + + let mut ctx = JobContext::new("Test", "non-object metadata"); + ctx.metadata = serde_json::json!("not an object"); + + let memory = crate::context::Memory::new(ctx.job_id); + let fb = FallbackDeliverable::build(&ctx, &memory, "failed"); + + store_fallback_in_metadata(&mut ctx, Some(&fb)); + + // Must normalize to object and store + assert!(ctx.metadata.is_object()); // safety: test + assert!(ctx.metadata.get("fallback_deliverable").is_some()); // safety: test + } + + #[test] + fn test_store_fallback_none_is_noop() { + let mut ctx = JobContext::new("Test", "noop"); + let original = ctx.metadata.clone(); + + store_fallback_in_metadata(&mut ctx, None); + + assert_eq!(ctx.metadata, original); // safety: test + } } From c4ab382522c86e7e19d55fee760b125fb1970518 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Thu, 19 Mar 2026 15:50:54 -0700 Subject: [PATCH 3/3] Make hosted OAuth and MCP auth generic (#1375) * Make hosted OAuth and MCP auth generic * Address PR feedback and lint issues * Suppress built-in Google secret in hosted proxy flows * Align hosted OAuth secret suppression with proxy config * Harden hosted OAuth callback helpers * Tighten hosted OAuth URL rewriting --- FEATURE_PARITY.md | 2 +- src/channels/web/server.rs | 163 ++++++-- src/cli/oauth_defaults.rs | 338 ++++++++++++---- src/cli/tool.rs | 21 +- src/extensions/manager.rs | 464 +++++++++++++++++----- src/llm/oauth_helpers.rs | 7 +- tests/e2e/mock_llm.py | 18 +- tests/e2e/scenarios/test_mcp_auth_flow.py | 4 + 8 files changed, 785 insertions(+), 232 deletions(-) diff --git a/FEATURE_PARITY.md b/FEATURE_PARITY.md index 85348de5..e0002a41 100644 --- a/FEATURE_PARITY.md +++ b/FEATURE_PARITY.md @@ -465,7 +465,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O | Device pairing | ✅ | ❌ | | | Tailscale identity | ✅ | ❌ | | | Trusted-proxy auth | ✅ | ❌ | Header-based reverse proxy auth | -| OAuth flows | ✅ | 🚧 | NEAR AI OAuth | +| OAuth flows | ✅ | 🚧 | NEAR AI OAuth plus hosted extension/MCP OAuth broker; external auth-proxy rollout still pending | | DM pairing verification | ✅ | ✅ | ironclaw pairing approve, host APIs | | Allowlist/blocklist | ✅ | 🚧 | allow_from + pairing store | | Per-group tool policies | ✅ | ❌ | | diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index ab697951..ea3341c0 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -19,6 +19,7 @@ use axum::{ routing::{get, post}, }; use serde::Deserialize; +use sha2::{Digest, Sha256}; use tokio::sync::{mpsc, oneshot}; use tokio_stream::StreamExt; use tower_http::cors::{AllowHeaders, CorsLayer}; @@ -63,6 +64,16 @@ pub type PromptQueue = Arc< pub type RoutineEngineSlot = Arc>>>; +fn redact_oauth_state_for_logs(state: &str) -> String { + let digest = Sha256::digest(state.as_bytes()); + let mut short_hash = String::with_capacity(12); + for byte in &digest[..6] { + use std::fmt::Write as _; + let _ = write!(&mut short_hash, "{byte:02x}"); + } + format!("sha256:{short_hash}:len={}", state.len()) +} + /// Simple sliding-window rate limiter. /// /// Tracks the number of requests in the current window. Resets when the window expires. @@ -566,22 +577,35 @@ async fn oauth_callback_handler( } }; - // Strip instance prefix from state for registry lookup. - // Platform nginx sends `state=instance:nonce` but flows are keyed by nonce only. - let lookup_key = oauth_defaults::strip_instance_prefix(&state_param); + let decoded_state = match oauth_defaults::decode_hosted_oauth_state(&state_param) { + Ok(decoded) => decoded, + Err(error) => { + let redacted_state = redact_oauth_state_for_logs(&state_param); + tracing::warn!( + state = %redacted_state, + error = %error, + "OAuth callback received with malformed state" + ); + clear_auth_mode(&state).await; + return oauth_error_page("IronClaw"); + } + }; + let lookup_key = decoded_state.flow_id.clone(); let flow = ext_mgr .pending_oauth_flows() .write() .await - .remove(lookup_key); + .remove(&lookup_key); let flow = match flow { Some(f) => f, None => { + let redacted_state = redact_oauth_state_for_logs(&state_param); + let redacted_lookup_key = redact_oauth_state_for_logs(&lookup_key); tracing::warn!( - state = %state_param, - lookup_key = %lookup_key, + state = %redacted_state, + lookup_key = %redacted_lookup_key, "OAuth callback received with unknown or expired state" ); clear_auth_mode(&state).await; @@ -608,33 +632,29 @@ async fn oauth_callback_handler( } // Exchange the authorization code for tokens. - // Use the platform exchange proxy when configured (keeps client_secret off container), - // otherwise call the provider's token URL directly. - let exchange_proxy_url = std::env::var("IRONCLAW_OAUTH_EXCHANGE_URL").ok(); + // Use the platform exchange proxy when configured, otherwise call the + // provider's token URL directly. + let exchange_proxy_url = oauth_defaults::exchange_proxy_url(); let result: Result<(), String> = async { - let token_response = if let (Some(proxy_url), None) = (&exchange_proxy_url, &flow.resource) - { - // Use the platform exchange proxy when configured and no resource - // parameter is needed. The proxy holds client_secret server-side so - // the container never sees it. MCP flows (resource.is_some()) bypass - // the proxy because it doesn't forward the RFC 8707 resource param. + let token_response = if let Some(proxy_url) = &exchange_proxy_url { let gateway_token = flow.gateway_token.as_deref().unwrap_or_default(); - oauth_defaults::exchange_via_proxy( + oauth_defaults::exchange_via_proxy(oauth_defaults::ProxyTokenExchangeRequest { proxy_url, gateway_token, - &code, - &flow.redirect_uri, - flow.code_verifier.as_deref(), - &flow.access_token_field, - ) + token_url: &flow.token_url, + client_id: &flow.client_id, + client_secret: flow.client_secret.as_deref(), + code: &code, + redirect_uri: &flow.redirect_uri, + code_verifier: flow.code_verifier.as_deref(), + access_token_field: &flow.access_token_field, + extra_token_params: &flow.token_exchange_extra_params, + }) .await .map_err(|e| e.to_string())? } else { - // Direct token exchange: uses exchange_oauth_code_with_resource so MCP - // flows can include the RFC 8707 `resource` parameter to scope the - // issued token to the specific MCP server. - oauth_defaults::exchange_oauth_code_with_resource( + oauth_defaults::exchange_oauth_code_with_params( &flow.token_url, &flow.client_id, flow.client_secret.as_deref(), @@ -642,7 +662,7 @@ async fn oauth_callback_handler( &flow.redirect_uri, flow.code_verifier.as_deref(), &flow.access_token_field, - flow.resource.as_deref(), + &flow.token_exchange_extra_params, ) .await .map_err(|e| e.to_string())? @@ -669,10 +689,8 @@ async fn oauth_callback_handler( .await .map_err(|e| e.to_string())?; - // For MCP OAuth flows (identified by resource field), persist the - // client_id so token refresh works without re-authentication. - // The CLI flow stores this in authorize_mcp_server(); the gateway - // callback must do the same. + // Persist the client_id for flows that need it after the session ends + // (for example DCR-based MCP refresh). if let Some(ref client_id_secret) = flow.client_id_secret_name { let params = crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id) .with_provider(flow.provider.as_ref().cloned().unwrap_or_default()); @@ -3311,7 +3329,7 @@ mod tests { secrets, sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at, }; @@ -3379,7 +3397,7 @@ mod tests { secrets, sse_sender: Some(sender), gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at, }; @@ -3482,7 +3500,7 @@ mod tests { secrets, sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, // Expired — handler will reject after lookup (no network I/O) created_at, @@ -3534,6 +3552,85 @@ mod tests { ); } + #[tokio::test] + async fn test_oauth_callback_accepts_versioned_hosted_state() { + use axum::body::Body; + use tower::ServiceExt; + + let secrets: Arc = + Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new( + crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( + TEST_GATEWAY_CRYPTO_KEY.to_string(), + )) + .expect("crypto"), + ))); + let (ext_mgr, _wasm_tools_dir, _wasm_channels_dir) = test_ext_mgr(secrets.clone()); + + let Some(created_at) = expired_flow_created_at() else { + eprintln!("Skipping versioned OAuth state test: monotonic uptime below expiry window"); + return; + }; + let flow = crate::cli::oauth_defaults::PendingOAuthFlow { + extension_name: "test_tool".to_string(), + display_name: "Test Tool".to_string(), + token_url: "https://example.com/token".to_string(), + client_id: "client123".to_string(), + client_secret: None, + redirect_uri: "https://example.com/oauth/callback".to_string(), + code_verifier: None, + access_token_field: "access_token".to_string(), + secret_name: "test_token".to_string(), + provider: None, + validation_endpoint: None, + scopes: vec![], + user_id: "test".to_string(), + secrets, + sse_sender: None, + gateway_token: None, + token_exchange_extra_params: std::collections::HashMap::new(), + client_id_secret_name: None, + created_at, + }; + + ext_mgr + .pending_oauth_flows() + .write() + .await + .insert("test_nonce".to_string(), flow); + + let state = test_gateway_state(Some(ext_mgr.clone())); + let app = test_oauth_router(state); + let versioned_state = + crate::cli::oauth_defaults::encode_hosted_oauth_state("test_nonce", Some("myinstance")); + + let req = axum::http::Request::builder() + .uri(format!( + "/oauth/callback?code=fake_code&state={}", + urlencoding::encode(&versioned_state) + )) + .body(Body::empty()) + .expect("request"); + + let resp = ServiceExt::>::oneshot(app, req) + .await + .expect("response"); + assert_eq!(resp.status(), StatusCode::OK); + + let body = axum::body::to_bytes(resp.into_body(), 1024 * 64) + .await + .expect("body"); + let html = String::from_utf8_lossy(&body); + assert!(html.contains("Authorization Failed")); + assert!( + ext_mgr + .pending_oauth_flows() + .read() + .await + .get("test_nonce") + .is_none() + ); + } + // --- Slack relay OAuth CSRF tests --- fn test_relay_oauth_router(state: Arc) -> Router { diff --git a/src/cli/oauth_defaults.rs b/src/cli/oauth_defaults.rs index a625f718..874cff98 100644 --- a/src/cli/oauth_defaults.rs +++ b/src/cli/oauth_defaults.rs @@ -5,17 +5,10 @@ //! //! # Built-in Credentials //! -//! Many CLI tools (gcloud, rclone, gdrive) ship with default OAuth credentials -//! so users don't need to register their own OAuth app. Google explicitly -//! documents that client_secret for "Desktop App" / "Installed App" types -//! is NOT actually secret. -//! -//! Default credentials are hardcoded below. They can be overridden at: -//! -//! - **Compile time**: Set IRONCLAW_GOOGLE_CLIENT_ID / IRONCLAW_GOOGLE_CLIENT_SECRET -//! env vars before building to replace the hardcoded defaults. -//! - **Runtime**: Users can set GOOGLE_OAUTH_CLIENT_ID / GOOGLE_OAUTH_CLIENT_SECRET -//! env vars, which take priority over built-in defaults. +//! Some providers ship with built-in OAuth credentials so users don't need to +//! register their own OAuth app just to get started. Today this module only +//! includes built-in defaults for Google-family tools, and those defaults can +//! be overridden by provider-specific environment variables when needed. use std::collections::HashMap; use std::sync::Arc; @@ -23,6 +16,7 @@ use std::time::Duration; use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; use rand::RngCore; +use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use tokio::sync::RwLock; @@ -60,6 +54,14 @@ pub fn builtin_credentials(secret_name: &str) -> Option { } } +/// Returns the compile-time override env var name, if this provider supports one. +pub fn builtin_client_id_override_env(secret_name: &str) -> Option<&'static str> { + match secret_name { + "google_oauth_token" => Some("IRONCLAW_GOOGLE_CLIENT_ID"), + _ => None, + } +} + // ── Shared callback server ────────────────────────────────────────────── // Core OAuth callback infrastructure is defined in `crate::llm::oauth_helpers` @@ -173,9 +175,8 @@ pub async fn exchange_oauth_code( code_verifier: Option<&str>, access_token_field: &str, ) -> Result { - // Delegates to exchange_oauth_code_with_resource with resource=None. - // Non-MCP OAuth flows don't need the RFC 8707 resource parameter. - exchange_oauth_code_with_resource( + let extra_token_params = HashMap::new(); + exchange_oauth_code_with_params( token_url, client_id, client_secret, @@ -183,16 +184,14 @@ pub async fn exchange_oauth_code( redirect_uri, code_verifier, access_token_field, - None, + &extra_token_params, ) .await } -/// Exchange an OAuth authorization code for tokens, with optional RFC 8707 `resource` parameter. -/// -/// The `resource` parameter scopes the issued token to a specific server (used by MCP OAuth). +/// Exchange an OAuth authorization code for tokens with generic extra form parameters. #[allow(clippy::too_many_arguments)] -pub async fn exchange_oauth_code_with_resource( +pub async fn exchange_oauth_code_with_params( token_url: &str, client_id: &str, client_secret: Option<&str>, @@ -200,7 +199,7 @@ pub async fn exchange_oauth_code_with_resource( redirect_uri: &str, code_verifier: Option<&str>, access_token_field: &str, - resource: Option<&str>, + extra_token_params: &HashMap, ) -> Result { let client = reqwest::Client::new(); let mut token_params = vec![ @@ -213,10 +212,8 @@ pub async fn exchange_oauth_code_with_resource( token_params.push(("code_verifier", verifier.to_string())); } - // RFC 8707: include the `resource` parameter so the authorization server - // scopes the issued token to the specific MCP server (protected resource). - if let Some(resource) = resource { - token_params.push(("resource", resource.to_string())); + for (key, value) in extra_token_params { + token_params.push((key.as_str(), value.clone())); } let mut request = client.post(token_url); @@ -276,6 +273,37 @@ pub async fn exchange_oauth_code_with_resource( }) } +/// Exchange an OAuth authorization code for tokens, with optional RFC 8707 `resource` parameter. +/// +/// The `resource` parameter scopes the issued token to a specific server (used by MCP OAuth). +#[allow(clippy::too_many_arguments)] +pub async fn exchange_oauth_code_with_resource( + token_url: &str, + client_id: &str, + client_secret: Option<&str>, + code: &str, + redirect_uri: &str, + code_verifier: Option<&str>, + access_token_field: &str, + resource: Option<&str>, +) -> Result { + let mut extra_token_params = HashMap::new(); + if let Some(resource) = resource { + extra_token_params.insert("resource".to_string(), resource.to_string()); + } + exchange_oauth_code_with_params( + token_url, + client_id, + client_secret, + code, + redirect_uri, + code_verifier, + access_token_field, + &extra_token_params, + ) + .await +} + /// Store OAuth tokens (access + refresh) in the secrets store. /// /// Also stores the granted scopes as `{secret_name}_scopes` so that scope @@ -423,9 +451,9 @@ pub struct PendingOAuthFlow { pub sse_sender: Option>, /// Gateway auth token for authenticating with the platform token exchange proxy. pub gateway_token: Option, - /// RFC 8707 resource parameter (MCP OAuth only). - /// Sent during token exchange to scope the token to a specific MCP server. - pub resource: Option, + /// Additional form params for the token exchange request. + /// Used for provider-specific requirements such as RFC 8707 `resource`. + pub token_exchange_extra_params: HashMap, /// Secret name for persisting the client ID (MCP OAuth only). /// Needed so token refresh can find the client_id after the session ends. pub client_id_secret_name: Option, @@ -459,9 +487,7 @@ pub fn new_pending_oauth_registry() -> PendingOAuthRegistry { /// URL, meaning the user's browser will redirect to a hosted gateway rather than /// localhost. pub fn use_gateway_callback() -> bool { - std::env::var("IRONCLAW_OAUTH_CALLBACK_URL") - .ok() - .filter(|v| !v.is_empty()) + crate::config::helpers::env_or_override("IRONCLAW_OAUTH_CALLBACK_URL") .map(|raw| { url::Url::parse(&raw) .ok() @@ -472,6 +498,13 @@ pub fn use_gateway_callback() -> bool { .unwrap_or(false) } +/// Returns the configured OAuth token-exchange proxy URL, if any. +pub fn exchange_proxy_url() -> Option { + crate::config::helpers::env_or_override("IRONCLAW_OAUTH_EXCHANGE_URL") + .map(|url| url.trim().to_string()) + .filter(|url| !url.is_empty()) +} + /// Maximum age for pending OAuth flows (5 minutes, matching TCP listener timeout). pub const OAUTH_FLOW_EXPIRY: Duration = Duration::from_secs(300); @@ -486,23 +519,117 @@ pub async fn sweep_expired_flows(registry: &PendingOAuthRegistry) { // ── Platform routing helpers ──────────────────────────────────────── -/// Prepend instance name to CSRF state for platform routing. +const HOSTED_STATE_PREFIX: &str = "ic2"; +const HOSTED_STATE_CHECKSUM_BYTES: usize = 12; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DecodedHostedOAuthState { + pub flow_id: String, + pub instance_name: Option, + pub is_legacy: bool, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct HostedOAuthStatePayload { + flow_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + instance_name: Option, + issued_at: u64, +} + +fn current_instance_name() -> Option { + crate::config::helpers::env_or_override("IRONCLAW_INSTANCE_NAME") + .or_else(|| crate::config::helpers::env_or_override("OPENCLAW_INSTANCE_NAME")) + .filter(|v| !v.is_empty()) +} + +fn hosted_state_checksum(payload_bytes: &[u8]) -> String { + let digest = Sha256::digest(payload_bytes); + URL_SAFE_NO_PAD.encode(&digest[..HOSTED_STATE_CHECKSUM_BYTES]) +} + +/// Build a versioned hosted OAuth state envelope. /// -/// The NEAR AI platform nginx proxy at `auth.DOMAIN` parses the instance name -/// from the `state` query parameter (format: `instance:nonce`) to route the -/// OAuth callback to the correct container. -/// -/// Returns the nonce unchanged when `IRONCLAW_INSTANCE_NAME` is not set -/// (local/non-platform mode). -pub fn build_platform_state(nonce: &str) -> String { - let instance = std::env::var("IRONCLAW_INSTANCE_NAME") - .or_else(|_| std::env::var("OPENCLAW_INSTANCE_NAME")) - .ok() - .filter(|v| !v.is_empty()); - match instance { - Some(name) => format!("{}:{}", name, nonce), - None => nonce.to_string(), +/// The encoded value is opaque to providers and can be decoded by both +/// IronClaw and the external auth proxy for routing and callback lookup. +pub fn encode_hosted_oauth_state(flow_id: &str, instance_name: Option<&str>) -> String { + let payload = HostedOAuthStatePayload { + flow_id: flow_id.to_string(), + instance_name: instance_name + .map(str::trim) + .filter(|v| !v.is_empty()) + .map(str::to_string), + issued_at: std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(), + }; + let payload_json = match serde_json::to_vec(&payload) { + Ok(payload_json) => payload_json, + Err(error) => { + tracing::warn!(%error, flow_id, "Failed to serialize hosted OAuth state payload"); + return payload.flow_id; + } + }; + let payload = URL_SAFE_NO_PAD.encode(&payload_json); + let checksum = hosted_state_checksum(&payload_json); + format!("{HOSTED_STATE_PREFIX}.{payload}.{checksum}") +} + +/// Decode hosted OAuth state in either the new versioned format or the +/// legacy `instance:nonce`/`nonce` forms. +pub fn decode_hosted_oauth_state(state: &str) -> Result { + if let Some(rest) = state.strip_prefix(&format!("{HOSTED_STATE_PREFIX}.")) + && let Some((payload_b64, checksum)) = rest.rsplit_once('.') + && let Ok(payload_json) = URL_SAFE_NO_PAD.decode(payload_b64) + { + let expected_checksum = hosted_state_checksum(&payload_json); + if checksum != expected_checksum { + return Err("Hosted OAuth state checksum mismatch".to_string()); + } + if let Ok(payload) = serde_json::from_slice::(&payload_json) + && !payload.flow_id.trim().is_empty() + { + return Ok(DecodedHostedOAuthState { + flow_id: payload.flow_id, + instance_name: payload.instance_name.filter(|v| !v.is_empty()), + is_legacy: false, + }); + } } + + if let Some((instance_name, flow_id)) = state.split_once(':') { + if flow_id.is_empty() { + return Err("Hosted OAuth legacy state is missing flow_id".to_string()); + } + return Ok(DecodedHostedOAuthState { + flow_id: flow_id.to_string(), + instance_name: if instance_name.is_empty() { + None + } else { + Some(instance_name.to_string()) + }, + is_legacy: true, + }); + } + + if state.is_empty() { + return Err("Hosted OAuth state is empty".to_string()); + } + + Ok(DecodedHostedOAuthState { + flow_id: state.to_string(), + instance_name: None, + is_legacy: true, + }) +} + +/// Build the hosted callback state used by the public OAuth callback endpoint. +/// +/// New flows emit a versioned opaque envelope, while callback decoding accepts +/// both the envelope and the legacy `instance:nonce` contract. +pub fn build_platform_state(nonce: &str) -> String { + encode_hosted_oauth_state(nonce, current_instance_name().as_deref()) } /// Strip the instance prefix from a state parameter to recover the lookup nonce. @@ -517,43 +644,62 @@ pub fn strip_instance_prefix(state: &str) -> &str { .unwrap_or(state) } +pub struct ProxyTokenExchangeRequest<'a> { + pub proxy_url: &'a str, + pub gateway_token: &'a str, + pub token_url: &'a str, + pub client_id: &'a str, + pub client_secret: Option<&'a str>, + pub code: &'a str, + pub redirect_uri: &'a str, + pub code_verifier: Option<&'a str>, + pub access_token_field: &'a str, + pub extra_token_params: &'a HashMap, +} + /// Exchange an OAuth authorization code via the platform's token exchange proxy. /// -/// The proxy holds `client_secret` server-side so the container never sees it. -/// Authenticated via the gateway auth token (Bearer header). +/// Authenticated via the gateway auth token (Bearer header). The caller may +/// either rely on proxy-side secret lookup or forward a `client_secret` when +/// the provider requires it. /// -/// The proxy expects form params `{code, redirect_uri, code_verifier}` and -/// returns a standard Google token response `{access_token, refresh_token, expires_in}`. +/// The proxy expects standard OAuth form params plus optional provider-specific +/// token params and returns a standard token response such as +/// `{access_token, refresh_token, expires_in}`. pub async fn exchange_via_proxy( - proxy_url: &str, - gateway_token: &str, - code: &str, - redirect_uri: &str, - code_verifier: Option<&str>, - access_token_field: &str, + request: ProxyTokenExchangeRequest<'_>, ) -> Result { - if gateway_token.is_empty() { + if request.gateway_token.is_empty() { return Err(OAuthCallbackError::Io( "Gateway auth token is required for proxy token exchange".to_string(), )); } - let exchange_url = format!("{}/oauth/exchange", proxy_url.trim_end_matches('/')); + let exchange_url = format!("{}/oauth/exchange", request.proxy_url.trim_end_matches('/')); let client = reqwest::Client::builder() .timeout(Duration::from_secs(60)) .build() .map_err(|e| OAuthCallbackError::Io(format!("Failed to build HTTP client: {}", e)))?; let mut params = vec![ - ("code", code.to_string()), - ("redirect_uri", redirect_uri.to_string()), + ("code", request.code.to_string()), + ("redirect_uri", request.redirect_uri.to_string()), + ("token_url", request.token_url.to_string()), + ("client_id", request.client_id.to_string()), + ("access_token_field", request.access_token_field.to_string()), ]; - if let Some(verifier) = code_verifier { + if let Some(verifier) = request.code_verifier { params.push(("code_verifier", verifier.to_string())); } + if let Some(secret) = request.client_secret { + params.push(("client_secret", secret.to_string())); + } + for (key, value) in request.extra_token_params { + params.push((key.as_str(), value.clone())); + } let response = client .post(&exchange_url) - .bearer_auth(gateway_token) + .bearer_auth(request.gateway_token) .form(¶ms) .send() .await @@ -576,7 +722,7 @@ pub async fn exchange_via_proxy( .map_err(|e| OAuthCallbackError::Io(format!("Failed to parse proxy response: {}", e)))?; let access_token = token_data - .get(access_token_field) + .get(request.access_token_field) .and_then(|v| v.as_str()) .ok_or_else(|| { let fields: Vec<&str> = token_data @@ -585,7 +731,7 @@ pub async fn exchange_via_proxy( .unwrap_or_default(); OAuthCallbackError::Io(format!( "No '{}' field in proxy response (fields present: {:?})", - access_token_field, fields + request.access_token_field, fields )) })? .to_string(); @@ -605,14 +751,10 @@ pub async fn exchange_via_proxy( #[cfg(test)] mod tests { - use std::sync::Mutex; - use crate::cli::oauth_defaults::{ builtin_credentials, callback_host, callback_url, is_loopback_host, landing_html, }; - - /// Serializes env-mutating tests to prevent parallel races. - static ENV_MUTEX: Mutex<()> = Mutex::new(()); + use crate::config::helpers::ENV_MUTEX; #[test] fn test_is_loopback_host() { @@ -935,7 +1077,7 @@ mod tests { #[test] fn test_build_platform_state_with_instance() { - use crate::cli::oauth_defaults::build_platform_state; + use crate::cli::oauth_defaults::{build_platform_state, decode_hosted_oauth_state}; let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_INSTANCE_NAME").ok(); @@ -943,7 +1085,11 @@ mod tests { unsafe { std::env::set_var("IRONCLAW_INSTANCE_NAME", "kind-deer"); } - assert_eq!(build_platform_state("abc123"), "kind-deer:abc123"); + let encoded = build_platform_state("abc123"); + let decoded = decode_hosted_oauth_state(&encoded).expect("decode hosted state"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name.as_deref(), Some("kind-deer")); + assert!(!decoded.is_legacy); unsafe { if let Some(val) = original { std::env::set_var("IRONCLAW_INSTANCE_NAME", val); @@ -955,7 +1101,7 @@ mod tests { #[test] fn test_build_platform_state_without_instance() { - use crate::cli::oauth_defaults::build_platform_state; + use crate::cli::oauth_defaults::{build_platform_state, decode_hosted_oauth_state}; let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_INSTANCE_NAME").ok(); @@ -965,7 +1111,11 @@ mod tests { std::env::remove_var("IRONCLAW_INSTANCE_NAME"); std::env::remove_var("OPENCLAW_INSTANCE_NAME"); } - assert_eq!(build_platform_state("abc123"), "abc123"); + let encoded = build_platform_state("abc123"); + let decoded = decode_hosted_oauth_state(&encoded).expect("decode hosted state"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name, None); + assert!(!decoded.is_legacy); unsafe { if let Some(val) = original { std::env::set_var("IRONCLAW_INSTANCE_NAME", val); @@ -978,7 +1128,7 @@ mod tests { #[test] fn test_build_platform_state_with_openclaw_instance() { - use crate::cli::oauth_defaults::build_platform_state; + use crate::cli::oauth_defaults::{build_platform_state, decode_hosted_oauth_state}; let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); let original_ic = std::env::var("IRONCLAW_INSTANCE_NAME").ok(); @@ -988,7 +1138,11 @@ mod tests { std::env::remove_var("IRONCLAW_INSTANCE_NAME"); std::env::set_var("OPENCLAW_INSTANCE_NAME", "quiet-lion"); } - assert_eq!(build_platform_state("xyz789"), "quiet-lion:xyz789"); + let encoded = build_platform_state("xyz789"); + let decoded = decode_hosted_oauth_state(&encoded).expect("decode hosted state"); + assert_eq!(decoded.flow_id, "xyz789"); + assert_eq!(decoded.instance_name.as_deref(), Some("quiet-lion")); + assert!(!decoded.is_legacy); unsafe { if let Some(val) = original_ic { std::env::set_var("IRONCLAW_INSTANCE_NAME", val); @@ -1017,6 +1171,42 @@ mod tests { assert_eq!(strip_instance_prefix(""), ""); } + #[test] + fn test_decode_hosted_oauth_state_accepts_legacy_formats() { + use crate::cli::oauth_defaults::decode_hosted_oauth_state; + + let decoded = decode_hosted_oauth_state("kind-deer:abc123").expect("legacy prefixed"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name.as_deref(), Some("kind-deer")); + assert!(decoded.is_legacy); + + let decoded = decode_hosted_oauth_state("abc123").expect("legacy raw"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name, None); + assert!(decoded.is_legacy); + } + + #[test] + fn test_decode_hosted_oauth_state_falls_back_for_non_envelope_ic2_prefix() { + use crate::cli::oauth_defaults::decode_hosted_oauth_state; + + let decoded = + decode_hosted_oauth_state("ic2.provider-owned-state").expect("prefixed fallback"); + assert_eq!(decoded.flow_id, "ic2.provider-owned-state"); + assert_eq!(decoded.instance_name, None); + assert!(decoded.is_legacy); + } + + #[test] + fn test_decode_hosted_oauth_state_rejects_tampered_checksum() { + use crate::cli::oauth_defaults::{decode_hosted_oauth_state, encode_hosted_oauth_state}; + + let encoded = encode_hosted_oauth_state("abc123", Some("kind-deer")); + let tampered = format!("{encoded}broken"); + let err = decode_hosted_oauth_state(&tampered).expect_err("tampered state should fail"); + assert!(err.contains("checksum"), "unexpected error: {err}"); + } + /// Verify that `build_oauth_url` includes the RFC 8707 `resource` parameter /// when passed through `extra_params`, which is how MCP OAuth gateway mode /// scopes tokens to a specific MCP server. diff --git a/src/cli/tool.rs b/src/cli/tool.rs index ac5d1b37..be684580 100644 --- a/src/cli/tool.rs +++ b/src/cli/tool.rs @@ -651,8 +651,8 @@ async fn auth_tool(name: String, dir: Option, user_id: String) -> anyho // Check for OAuth configuration if let Some(ref oauth) = auth.oauth { - // For providers with shared tokens (e.g., all Google tools share google_oauth_token), - // combine scopes from all installed tools so one auth covers everything. + // For providers with shared tokens, combine scopes from all installed + // tools so one auth covers everything. let combined = combine_provider_scopes(&tools_dir, &auth.secret_name, oauth).await; if combined.scopes.len() > oauth.scopes.len() { let extra = combined.scopes.len() - oauth.scopes.len(); @@ -670,8 +670,8 @@ async fn auth_tool(name: String, dir: Option, user_id: String) -> anyho } /// Scan the tools directory for all capabilities files sharing the same secret_name -/// and combine their OAuth scopes. This way, authing any Google tool requests scopes -/// for ALL installed Google tools, so one login covers everything. +/// and combine their OAuth scopes so one authorization covers the full shared +/// credential set. async fn combine_provider_scopes( tools_dir: &Path, secret_name: &str, @@ -736,11 +736,18 @@ async fn auth_tool_oauth( }) .or_else(|| builtin.as_ref().map(|c| c.client_id.to_string())) .ok_or_else(|| { - anyhow::anyhow!( + let mut message = format!( "OAuth client_id not configured.\n\ - Set {} env var, or build with IRONCLAW_GOOGLE_CLIENT_ID.", + Set {} env var", oauth.client_id_env.as_deref().unwrap_or("the client_id") - ) + ); + if let Some(override_env) = + oauth_defaults::builtin_client_id_override_env(&auth.secret_name) + { + message.push_str(&format!(", or build with {override_env}")); + } + message.push('.'); + anyhow::anyhow!(message) })?; // Get client_secret: capabilities file > runtime env var > built-in defaults diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index fbc06d5d..0762f3ed 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -45,6 +45,56 @@ struct PendingAuth { task_handle: Option>, } +struct HostedOAuthFlowStart { + name: String, + kind: ExtensionKind, + auth_url: String, + expected_state: String, + flow: crate::cli::oauth_defaults::PendingOAuthFlow, +} + +fn hosted_proxy_client_secret( + client_secret: &Option, + builtin: Option<&crate::cli::oauth_defaults::OAuthCredentials>, + exchange_proxy_configured: bool, +) -> Option { + if !exchange_proxy_configured { + return client_secret.clone(); + } + + let builtin_secret = builtin.map(|credentials| credentials.client_secret); + match (client_secret, builtin_secret) { + (Some(resolved), Some(baked_in)) if resolved == baked_in => None, + _ => client_secret.clone(), + } +} + +fn normalize_oauth_callback_path(path: &str) -> String { + let trimmed_path = path.trim_end_matches('/'); + if trimmed_path.is_empty() { + "/oauth/callback".to_string() + } else if trimmed_path.ends_with("/oauth/callback") { + trimmed_path.to_string() + } else { + format!("{trimmed_path}/oauth/callback") + } +} + +fn normalize_hosted_callback_url(callback_url: &str) -> String { + if let Ok(mut parsed) = url::Url::parse(callback_url) { + let normalized_path = normalize_oauth_callback_path(parsed.path()); + parsed.set_path(&normalized_path); + return parsed.to_string(); + } + + let normalized_callback_url = callback_url.trim_end_matches('/'); + if normalized_callback_url.ends_with("/oauth/callback") { + normalized_callback_url.to_string() + } else { + format!("{normalized_callback_url}/oauth/callback") + } +} + /// Runtime infrastructure needed for hot-activating WASM channels. /// /// Set after construction via [`ExtensionManager::set_channel_runtime`] once the @@ -547,7 +597,9 @@ impl ExtensionManager { async fn gateway_callback_redirect_uri(&self) -> Option { use crate::cli::oauth_defaults; if oauth_defaults::use_gateway_callback() { - return Some(format!("{}/oauth/callback", oauth_defaults::callback_url())); + return Some(normalize_hosted_callback_url( + &oauth_defaults::callback_url(), + )); } // Use gateway_base_url from enable_gateway_mode() if let Some(ref base) = *self.gateway_base_url.read().await { @@ -924,6 +976,98 @@ impl ExtensionManager { &self.pending_oauth_flows } + async fn clear_pending_extension_auth(&self, name: &str) { + { + let mut pending = self.pending_auth.write().await; + if let Some(old) = pending.remove(name) + && let Some(handle) = old.task_handle + { + handle.abort(); + } + } + + let mut flows = self.pending_oauth_flows.write().await; + flows.retain(|_, flow| flow.extension_name != name); + } + + fn rewrite_oauth_state_param( + auth_url: String, + expected_state: &str, + hosted_state: &str, + ) -> String { + if hosted_state == expected_state { + return auth_url; + } + + let Ok(mut parsed) = url::Url::parse(&auth_url) else { + return auth_url.replace( + &format!("state={}", urlencoding::encode(expected_state)), + &format!("state={}", urlencoding::encode(hosted_state)), + ); + }; + + let mut replaced = false; + let pairs: Vec<(String, String)> = parsed + .query_pairs() + .map(|(key, value)| { + if key == "state" { + replaced = true; + (key.into_owned(), hosted_state.to_string()) + } else { + (key.into_owned(), value.into_owned()) + } + }) + .collect(); + + { + let mut query_pairs = parsed.query_pairs_mut(); + query_pairs.clear(); + for (key, value) in pairs { + query_pairs.append_pair(&key, &value); + } + if !replaced { + query_pairs.append_pair("state", hosted_state); + } + } + + parsed.to_string() + } + + async fn start_gateway_oauth_flow(&self, request: HostedOAuthFlowStart) -> AuthResult { + use crate::cli::oauth_defaults; + + oauth_defaults::sweep_expired_flows(&self.pending_oauth_flows).await; + + let hosted_state = oauth_defaults::build_platform_state(&request.expected_state); + let auth_url = Self::rewrite_oauth_state_param( + request.auth_url, + &request.expected_state, + &hosted_state, + ); + + self.pending_oauth_flows + .write() + .await + .insert(request.expected_state, request.flow); + + self.pending_auth.write().await.insert( + request.name.clone(), + PendingAuth { + _name: request.name.clone(), + _kind: request.kind, + created_at: std::time::Instant::now(), + task_handle: None, + }, + ); + + AuthResult::awaiting_authorization( + request.name, + request.kind, + auth_url, + "gateway".to_string(), + ) + } + /// Broadcast an extension status change to the web UI via SSE. async fn broadcast_extension_status(&self, name: &str, status: &str, message: Option<&str>) { if let Some(ref sender) = *self.sse_sender.read().await { @@ -2383,6 +2527,7 @@ impl ExtensionManager { use crate::cli::oauth_defaults; let is_gateway = self.should_use_gateway_mode(); + self.clear_pending_extension_auth(name).await; // Build redirect URI: gateway uses the public callback URL, // local mode binds a random port. @@ -2440,19 +2585,8 @@ impl ExtensionManager { let code_verifier = oauth_result.code_verifier; if is_gateway { - // Gateway mode: store pending flow for the /oauth/callback handler. - oauth_defaults::sweep_expired_flows(&self.pending_oauth_flows).await; - - // Platform routing: prepend instance name to state - let platform_state = oauth_defaults::build_platform_state(&expected_state); - let auth_url = if platform_state != expected_state { - oauth_result.url.replace( - &format!("state={}", urlencoding::encode(&expected_state)), - &format!("state={}", urlencoding::encode(&platform_state)), - ) - } else { - oauth_result.url - }; + let mut token_exchange_extra_params = HashMap::new(); + token_exchange_extra_params.insert("resource".to_string(), resource.clone()); let flow = oauth_defaults::PendingOAuthFlow { extension_name: name.to_string(), @@ -2471,7 +2605,7 @@ impl ExtensionManager { secrets: Arc::clone(&self.secrets), sse_sender: self.sse_sender.read().await.clone(), gateway_token: self.gateway_token.clone(), - resource: Some(resource), + token_exchange_extra_params, client_id_secret_name: if server.oauth.is_none() { Some(server.client_id_secret_name()) } else { @@ -2480,27 +2614,15 @@ impl ExtensionManager { created_at: std::time::Instant::now(), }; - self.pending_oauth_flows - .write() - .await - .insert(expected_state, flow); - - self.pending_auth.write().await.insert( - name.to_string(), - PendingAuth { - _name: name.to_string(), - _kind: ExtensionKind::McpServer, - created_at: std::time::Instant::now(), - task_handle: None, - }, - ); - - Ok(AuthResult::awaiting_authorization( - name, - ExtensionKind::McpServer, - auth_url, - "gateway".to_string(), - )) + Ok(self + .start_gateway_oauth_flow(HostedOAuthFlowStart { + name: name.to_string(), + kind: ExtensionKind::McpServer, + auth_url: oauth_result.url, + expected_state, + flow, + }) + .await) } else { // Local mode: return URL for manual opening self.pending_auth.write().await.insert( @@ -2901,9 +3023,10 @@ impl ExtensionManager { Enter it in the Setup tab or set {} env var", name, env_name ); - // Only mention the Google-specific build flag for Google providers - if auth.secret_name.to_lowercase().contains("google") { - msg.push_str(", or build with IRONCLAW_GOOGLE_CLIENT_ID"); + if let Some(override_env) = + crate::cli::oauth_defaults::builtin_client_id_override_env(&auth.secret_name) + { + msg.push_str(&format!(", or build with {override_env}")); } msg.push('.'); msg @@ -2919,20 +3042,7 @@ impl ExtensionManager { ) .await; - // Cancel any existing pending auth for this tool (frees port 9876 in TCP mode) - { - let mut pending = self.pending_auth.write().await; - if let Some(old) = pending.remove(name) - && let Some(handle) = old.task_handle - { - handle.abort(); - } - } - // Also clean up any gateway-mode pending flows for this tool - { - let mut flows = self.pending_oauth_flows.write().await; - flows.retain(|_, flow| flow.extension_name != name); - } + self.clear_pending_extension_auth(name).await; let redirect_uri = self .gateway_callback_redirect_uri() @@ -2963,30 +3073,24 @@ impl ExtensionManager { .unwrap_or_else(|| name.to_string()); if self.should_use_gateway_mode() { - // Gateway mode: store pending flow state for the web gateway's - // `/oauth/callback` handler to complete the exchange. No TCP listener - // needed — the OAuth provider redirects to the gateway URL. - oauth_defaults::sweep_expired_flows(&self.pending_oauth_flows).await; - - // Wrap the CSRF nonce with instance name for platform routing. - // Nginx at auth.DOMAIN parses `instance:nonce` to route the callback - // to the correct container. The flow is keyed by the raw nonce. - let platform_state = oauth_defaults::build_platform_state(&expected_state); - let auth_url = if platform_state != expected_state { - auth_url.replace( - &format!("state={}", urlencoding::encode(&expected_state)), - &format!("state={}", urlencoding::encode(&platform_state)), - ) - } else { - auth_url - }; + // When an exchange proxy is configured, omit the client_secret if it + // was resolved from built-in defaults (desktop app credentials). The + // proxy holds the correct web-app secret for platform-registered OAuth + // apps. Sending the desktop secret would cause a client_id/secret + // mismatch because the container's GOOGLE_OAUTH_CLIENT_ID is the web + // app, not the desktop app. + let proxy_client_secret = hosted_proxy_client_secret( + &client_secret, + builtin.as_ref(), + oauth_defaults::exchange_proxy_url().is_some(), + ); let flow = oauth_defaults::PendingOAuthFlow { extension_name: name.to_string(), display_name: display_name.clone(), token_url: oauth.token_url.clone(), client_id: client_id.clone(), - client_secret: client_secret.clone(), + client_secret: proxy_client_secret, redirect_uri: redirect_uri.clone(), code_verifier, access_token_field: oauth.access_token_field.clone(), @@ -2998,35 +3102,20 @@ impl ExtensionManager { secrets: Arc::clone(&self.secrets), sse_sender: self.sse_sender.read().await.clone(), gateway_token: self.gateway_token.clone(), - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at: std::time::Instant::now(), }; - // Key by raw nonce (without instance prefix) — the callback handler - // strips the prefix before lookup. - self.pending_oauth_flows - .write() - .await - .insert(expected_state, flow); - - // Register pending auth without a task handle (gateway handles completion) - self.pending_auth.write().await.insert( - name.to_string(), - PendingAuth { - _name: name.to_string(), - _kind: ExtensionKind::WasmTool, - created_at: std::time::Instant::now(), - task_handle: None, - }, - ); - - Ok(AuthResult::awaiting_authorization( - name, - ExtensionKind::WasmTool, - auth_url, - "gateway".to_string(), - )) + Ok(self + .start_gateway_oauth_flow(HostedOAuthFlowStart { + name: name.to_string(), + kind: ExtensionKind::WasmTool, + auth_url, + expected_state, + flow, + }) + .await) } else { // TCP listener mode: bind port 9876 and spawn a background task // to wait for the callback. This is the original flow for local/desktop use. @@ -5241,7 +5330,8 @@ mod tests { use crate::extensions::manager::{ ChannelRuntimeState, FallbackDecision, TelegramBindingData, TelegramBindingResult, TelegramOwnerBindingState, build_wasm_channel_runtime_config_updates, - combine_install_errors, fallback_decision, infer_kind_from_url, send_telegram_text_message, + combine_install_errors, fallback_decision, hosted_proxy_client_secret, infer_kind_from_url, + normalize_hosted_callback_url, send_telegram_text_message, telegram_message_matches_verification_code, }; use crate::extensions::{ @@ -6510,7 +6600,7 @@ mod tests { secrets: Arc::clone(&secrets), sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at: std::time::Instant::now(), }, @@ -6534,7 +6624,7 @@ mod tests { secrets, sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at: std::time::Instant::now(), }, @@ -6701,9 +6791,6 @@ mod tests { // The root cause was that `should_use_gateway_mode()` only checked the // `IRONCLAW_OAUTH_CALLBACK_URL` env var, ignoring `self.tunnel_url`. - /// Serializes env-mutating tests to prevent parallel races. - static GATEWAY_ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); - /// Build a minimal ExtensionManager with a custom tunnel_url. fn make_manager_with_tunnel(tunnel_url: Option) -> ExtensionManager { use crate::secrets::{InMemorySecretsStore, SecretsCrypto}; @@ -6736,9 +6823,11 @@ mod tests { #[test] fn should_use_gateway_mode_true_for_tunnel_url() { - let _guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); - // SAFETY: Under GATEWAY_ENV_MUTEX, no concurrent env access. + // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); } @@ -6758,7 +6847,9 @@ mod tests { #[test] fn should_use_gateway_mode_false_without_tunnel() { - let _guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); @@ -6779,7 +6870,9 @@ mod tests { #[test] fn should_use_gateway_mode_false_for_loopback_tunnel() { - let _guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); @@ -6807,9 +6900,11 @@ mod tests { impl EnvGuard { fn new() -> Self { - let guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); - // SAFETY: Under GATEWAY_ENV_MUTEX, no concurrent env access. + // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); } @@ -6822,7 +6917,7 @@ mod tests { impl Drop for EnvGuard { fn drop(&mut self) { - // SAFETY: Under GATEWAY_ENV_MUTEX (still held by _mutex), no concurrent env access. + // SAFETY: Under ENV_MUTEX (still held by _mutex), no concurrent env access. unsafe { if let Some(ref val) = self.original { std::env::set_var("IRONCLAW_OAUTH_CALLBACK_URL", val); @@ -6863,6 +6958,90 @@ mod tests { ); } + #[test] + fn gateway_callback_redirect_uri_does_not_duplicate_callback_path_from_env() { + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); + let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); + unsafe { + std::env::set_var( + "IRONCLAW_OAUTH_CALLBACK_URL", + "https://oauth.test.example/oauth/callback", + ); + } + + let mgr = make_manager_with_tunnel(None); + assert_eq!( + tokio_test::block_on(mgr.gateway_callback_redirect_uri()), + Some("https://oauth.test.example/oauth/callback".to_string()), + ); + + unsafe { + if let Some(val) = original { + std::env::set_var("IRONCLAW_OAUTH_CALLBACK_URL", val); + } else { + std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); + } + } + } + + #[test] + fn gateway_callback_redirect_uri_trims_trailing_slash_from_env_callback() { + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); + let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); + unsafe { + std::env::set_var( + "IRONCLAW_OAUTH_CALLBACK_URL", + "https://oauth.test.example/oauth/callback/", + ); + } + + let mgr = make_manager_with_tunnel(None); + assert_eq!( + tokio_test::block_on(mgr.gateway_callback_redirect_uri()), + Some("https://oauth.test.example/oauth/callback".to_string()), + ); + + unsafe { + if let Some(val) = original { + std::env::set_var("IRONCLAW_OAUTH_CALLBACK_URL", val); + } else { + std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); + } + } + } + + #[test] + fn normalize_hosted_callback_url_preserves_query_params() { + assert_eq!( + normalize_hosted_callback_url("https://oauth.test.example?source=hosted"), + "https://oauth.test.example/oauth/callback?source=hosted" + ); + assert_eq!( + normalize_hosted_callback_url( + "https://oauth.test.example/oauth/callback?source=hosted" + ), + "https://oauth.test.example/oauth/callback?source=hosted" + ); + } + + #[test] + fn rewrite_oauth_state_param_updates_only_state_query_param() { + let auth_url = + "https://auth.example.com/authorize?client_id=abc&state=old-state&hint=state%3Dkeep"; + assert_eq!( + ExtensionManager::rewrite_oauth_state_param( + auth_url.to_string(), + "old-state", + "new-hosted-state", + ), + "https://auth.example.com/authorize?client_id=abc&state=new-hosted-state&hint=state%3Dkeep" + ); + } + #[tokio::test] async fn gateway_mode_enabled_explicitly() { let _env = EnvGuard::new(); @@ -7217,4 +7396,71 @@ mod tests { panic!("URL missing token: {url}"); // safety: test assertion } } + + // ── proxy_client_secret suppression ───────────────────────────── + + #[test] + fn test_proxy_client_secret_suppressed_when_builtin_matches_with_exchange_proxy() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + let builtin_ref = builtin.as_ref(); + let secret = Some(builtin_ref.unwrap().client_secret.to_string()); + + let result = hosted_proxy_client_secret(&secret, builtin_ref, true); + assert_eq!( + result, None, + "built-in desktop secret must be suppressed when the exchange proxy is configured" + ); + } + + #[test] + fn test_proxy_client_secret_kept_when_not_builtin_with_exchange_proxy() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + let secret = Some("user-entered-custom-secret".to_string()); + + let result = hosted_proxy_client_secret(&secret, builtin.as_ref(), true); + assert_eq!( + result, + Some("user-entered-custom-secret".to_string()), + "non-builtin secret must be kept even when the exchange proxy is configured" + ); + } + + #[test] + fn test_proxy_client_secret_kept_without_exchange_proxy_even_for_builtin_secret() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + let builtin_ref = builtin.as_ref(); + let secret = Some(builtin_ref.unwrap().client_secret.to_string()); + + let result = hosted_proxy_client_secret(&secret, builtin_ref, false); + assert_eq!( + result, secret, + "built-in secret must be kept when the callback will exchange directly" + ); + } + + #[test] + fn test_proxy_client_secret_none_stays_none() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + + let result = hosted_proxy_client_secret(&None, builtin.as_ref(), true); + assert_eq!( + result, None, + "None secret stays None even when the exchange proxy is configured" + ); + } + + #[test] + fn test_proxy_client_secret_no_builtin_provider() { + // MCP/non-Google providers have no builtin credentials + let builtin = crate::cli::oauth_defaults::builtin_credentials("mcp_notion_access_token"); + assert!(builtin.is_none()); + + let secret = Some("dcr-secret".to_string()); + let result = hosted_proxy_client_secret(&secret, builtin.as_ref(), true); + assert_eq!( + result, + Some("dcr-secret".to_string()), + "non-builtin provider secret must be kept" + ); + } } diff --git a/src/llm/oauth_helpers.rs b/src/llm/oauth_helpers.rs index 551fc04b..b63457fd 100644 --- a/src/llm/oauth_helpers.rs +++ b/src/llm/oauth_helpers.rs @@ -39,9 +39,7 @@ pub enum OAuthCallbackError { /// deployments where `127.0.0.1` is unreachable from the user's browser), /// then falls back to `http://{callback_host()}:{OAUTH_CALLBACK_PORT}`. pub fn callback_url() -> String { - std::env::var("IRONCLAW_OAUTH_CALLBACK_URL") - .ok() - .filter(|v| !v.is_empty()) + crate::config::helpers::env_or_override("IRONCLAW_OAUTH_CALLBACK_URL") .unwrap_or_else(|| format!("http://{}:{}", callback_host(), OAUTH_CALLBACK_PORT)) } @@ -57,7 +55,8 @@ pub fn callback_url() -> String { /// Note: this transmits the session token over plain HTTP — prefer SSH port /// forwarding (`ssh -L 9876:127.0.0.1:9876 user@host`) when possible. pub fn callback_host() -> String { - std::env::var("OAUTH_CALLBACK_HOST").unwrap_or_else(|_| "127.0.0.1".to_string()) + crate::config::helpers::env_or_override("OAUTH_CALLBACK_HOST") + .unwrap_or_else(|| "127.0.0.1".to_string()) } /// Returns `true` if `host` is a loopback address that only accepts local connections. diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py index c27f2762..359c22d5 100644 --- a/tests/e2e/mock_llm.py +++ b/tests/e2e/mock_llm.py @@ -267,14 +267,24 @@ async def _stream_tool_call(request: web.Request, cid: str, tc: dict) -> web.Str async def oauth_exchange(request: web.Request) -> web.Response: """Mock OAuth token exchange proxy for E2E tests. - Accepts form params (code, redirect_uri, code_verifier) and returns - a fake token response. Called by ironclaw's exchange_via_proxy() when - IRONCLAW_OAUTH_EXCHANGE_URL is set. + Accepts the generic hosted OAuth proxy contract used by IronClaw and + returns a fake token response. MCP callback tests assert that provider- + specific token params such as RFC 8707 `resource` are forwarded here. """ data = await request.post() code = data.get("code", "") + access_token_field = data.get("access_token_field", "access_token") + + if code == "mock_mcp_code": + if not data.get("token_url", "").endswith("/oauth/token"): + return web.json_response({"error": "missing_token_url"}, status=400) + if not data.get("client_id"): + return web.json_response({"error": "missing_client_id"}, status=400) + if not data.get("resource"): + return web.json_response({"error": "missing_resource"}, status=400) + return web.json_response({ - "access_token": f"mock-token-{code}", + access_token_field: f"mock-token-{code}", "refresh_token": "mock-refresh-token", "expires_in": 3600, }) diff --git a/tests/e2e/scenarios/test_mcp_auth_flow.py b/tests/e2e/scenarios/test_mcp_auth_flow.py index 7de2bbe6..cc36aa2e 100644 --- a/tests/e2e/scenarios/test_mcp_auth_flow.py +++ b/tests/e2e/scenarios/test_mcp_auth_flow.py @@ -99,6 +99,10 @@ async def test_mcp_activate_triggers_auth(ironclaw_server): assert auth_url is not None or awaiting_token, ( f"Activate should require auth, got: {data}" ) + if auth_url is not None: + assert _extract_state(auth_url).startswith("ic2."), ( + f"Hosted MCP OAuth should emit versioned state, got: {auth_url}" + ) # ── Section C: OAuth Round-Trip ──────────────────────────────────────────