From 31c3b5b041f87909f74c6e5a1af6f64ce06f7d3f Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 19 Mar 2026 22:36:34 -0700 Subject: [PATCH 1/8] feat(agent): activate stuck_threshold for time-based stuck job detection (#1234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(agent): activate stuck_threshold for time-based stuck job detection (#1223) The stuck_threshold field on DefaultSelfRepair was defined but never used (marked #[allow(dead_code)]). Jobs that got stuck in InProgress without transitioning to Stuck state (e.g., deadlock, unhandled timeout) were never detected by self-repair. Changes: - Add find_stuck_jobs_with_threshold() to ContextManager that detects InProgress jobs running longer than the threshold - Wire stuck_threshold into detect_stuck_jobs() so it uses threshold-based detection alongside explicit Stuck state detection - Remove dead_code annotation from stuck_threshold - Accept InProgress jobs in the stuck job detection filter Configurable via AGENT_STUCK_THRESHOLD_SECS (default: 300s). Closes #1223 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(agent): address PR #1234 review feedback for stuck_threshold - Transition InProgress jobs to Stuck before returning them from detect_stuck_jobs(), so attempt_recovery() (which requires Stuck state) works correctly on threshold-detected jobs - Add detect-and-repair E2E test covering the full InProgress -> Stuck -> recovery -> InProgress cycle - Rename idle_threshold -> elapsed_threshold in find_stuck_jobs_with_threshold for clarity - Add `use std::time::Duration` import and remove fully qualified paths - Update CLAUDE.md to reflect that stuck_threshold is now actively used Co-Authored-By: Claude Opus 4.6 (1M context) * fix: measure stuck_duration from Stuck transition, handle InProgress→Stuck in repair - Fix stuck_duration computation to use the most recent Stuck transition timestamp instead of started_at, preventing jobs that ran for hours before becoming stuck from immediately exceeding the threshold - Fix last_activity to also use the Stuck transition timestamp - Transition InProgress jobs to Stuck before calling attempt_recovery() in repair_stuck_job(), since attempt_recovery() requires JobState::Stuck - Add regression test verifying a recently-stuck job with old started_at is not misdetected as exceeding a 5-minute threshold Co-Authored-By: Claude Opus 4.6 * fix(agent): address Copilot review comments on PR #1234 - Add comment in find_stuck_jobs_with_threshold() noting that started_at is not reset on Stuck->InProgress recovery, which may cause false positives for recovered jobs. Suggests tracking in_progress_since or using the most recent StateTransition as a future improvement. - Fix misleading test comment in stuck_duration_measured_from_stuck_transition test: explicitly Stuck jobs are always returned regardless of threshold. The test verifies stuck_duration is near-zero, not that the job is excluded. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: ilblackdragon@gmail.com --- src/agent/CLAUDE.md | 2 +- src/agent/self_repair.rs | 178 ++++++++++++++++++++++++++++++--------- src/context/manager.rs | 79 ++++++++++++++++- 3 files changed, 216 insertions(+), 43 deletions(-) diff --git a/src/agent/CLAUDE.md b/src/agent/CLAUDE.md index e55c9591..686753de 100644 --- a/src/agent/CLAUDE.md +++ b/src/agent/CLAUDE.md @@ -113,7 +113,7 @@ Check-insert is done under a single write lock to prevent TOCTOU races. A cleanu 4. Detects broken tools via `store.get_broken_tools(5)` (threshold: 5 failures). Requires `with_store()` to be called; returns empty without a store. 5. Attempts to rebuild broken tools via `SoftwareBuilder`. Requires `with_builder()` to be called; returns `ManualRequired` without a builder. -Note: the `stuck_threshold` duration is stored but currently unused (marked `#[allow(dead_code)]`). Stuck detection relies on `JobState::Stuck` being set by the state machine, not wall-clock time comparison. +The `stuck_threshold` duration is used for time-based detection of `InProgress` jobs that have been running longer than the threshold. When `detect_stuck_jobs()` finds such jobs, it transitions them to `Stuck` before returning them, enabling the normal `attempt_recovery()` path. Repair results: `Success`, `Retry`, `Failed`, `ManualRequired`. `Retry` does NOT notify the user (to avoid spam). diff --git a/src/agent/self_repair.rs b/src/agent/self_repair.rs index db491194..4e58cb15 100644 --- a/src/agent/self_repair.rs +++ b/src/agent/self_repair.rs @@ -66,6 +66,7 @@ pub trait SelfRepair: Send + Sync { /// Default self-repair implementation. pub struct DefaultSelfRepair { context_manager: Arc, + /// Jobs in `InProgress` longer than this are treated as stuck. stuck_threshold: Duration, max_repair_attempts: u32, store: Option>, @@ -111,15 +112,58 @@ impl DefaultSelfRepair { #[async_trait] impl SelfRepair for DefaultSelfRepair { async fn detect_stuck_jobs(&self) -> Vec { - let stuck_ids = self.context_manager.find_stuck_jobs().await; + let stuck_ids = self + .context_manager + .find_stuck_jobs_with_threshold(Some(self.stuck_threshold)) + .await; let mut stuck_jobs = Vec::new(); for job_id in stuck_ids { if let Ok(ctx) = self.context_manager.get_context(job_id).await - && ctx.state == JobState::Stuck + && matches!(ctx.state, JobState::Stuck | JobState::InProgress) { - // Measure stuck_duration from the most recent Stuck transition, - // not from started_at (which reflects when the job first ran). + // InProgress jobs detected by threshold need to be transitioned + // to Stuck before they can be repaired (attempt_recovery requires + // Stuck state). These jobs already passed the threshold check in + // find_stuck_jobs_with_threshold, so skip the duration filter below. + let just_transitioned = ctx.state == JobState::InProgress; + if just_transitioned { + let reason = "exceeded stuck_threshold"; + let transition = self + .context_manager + .update_context(job_id, |ctx| ctx.mark_stuck(reason)) + .await; + match transition { + Ok(Ok(())) => {} + Ok(Err(e)) => { + tracing::warn!( + job = %job_id, + "Failed to mark InProgress job as Stuck: {}", + e + ); + continue; + } + Err(e) => { + tracing::warn!( + job = %job_id, + "Failed to transition InProgress job to Stuck: {}", + e + ); + continue; + } + } + } + + // Re-fetch context after potential InProgress->Stuck transition + // so that stuck_since picks up the new transition timestamp. + let ctx = match self.context_manager.get_context(job_id).await { + Ok(c) => c, + Err(_) => continue, + }; + + // Use the timestamp of the most recent Stuck transition, not started_at. + // A job that ran for hours before becoming stuck should not immediately + // exceed the threshold — we measure from when it actually became stuck. let stuck_since = ctx .transitions .iter() @@ -134,8 +178,10 @@ impl SelfRepair for DefaultSelfRepair { }) .unwrap_or_default(); - // Only report jobs that have been stuck long enough - if stuck_duration < self.stuck_threshold { + // Only report already-Stuck jobs that have been stuck long enough. + // Jobs just transitioned from InProgress skip this check — they + // were already vetted by find_stuck_jobs_with_threshold. + if !just_transitioned && stuck_duration < self.stuck_threshold { continue; } @@ -163,10 +209,17 @@ impl SelfRepair for DefaultSelfRepair { }); } - // Try to recover the job + // Try to recover the job. + // If the job is still InProgress (detected via stuck_threshold), transition + // it to Stuck first so that attempt_recovery() can move it back to InProgress. let result = self .context_manager - .update_context(job.job_id, |ctx| ctx.attempt_recovery()) + .update_context(job.job_id, |ctx| { + if ctx.state == JobState::InProgress { + ctx.transition_to(JobState::Stuck, Some("exceeded stuck_threshold".into()))?; + } + ctx.attempt_recovery() + }) .await; match result { @@ -489,6 +542,82 @@ mod tests { ); } + #[tokio::test] + async fn detect_and_repair_in_progress_job_via_threshold() { + let cm = Arc::new(ContextManager::new(10)); + let job_id = cm.create_job("Long running", "desc").await.unwrap(); + + // Transition to InProgress. + cm.update_context(job_id, |ctx| ctx.transition_to(JobState::InProgress, None)) + .await + .unwrap() + .unwrap(); + + // Backdate started_at to simulate a job running for 10 minutes. + cm.update_context(job_id, |ctx| { + ctx.started_at = Some(Utc::now() - chrono::Duration::seconds(600)); + }) + .await + .unwrap(); + + // Use a 5-minute threshold so the 10-minute job is detected. + let repair = DefaultSelfRepair::new(Arc::clone(&cm), Duration::from_secs(300), 3); + + // detect_stuck_jobs should find it and transition InProgress -> Stuck. + let stuck = repair.detect_stuck_jobs().await; + assert_eq!(stuck.len(), 1); + assert_eq!(stuck[0].job_id, job_id); + + // After detection the job should now be in Stuck state. + let ctx = cm.get_context(job_id).await.unwrap(); + assert_eq!(ctx.state, JobState::Stuck); + + // Repair should recover it: Stuck -> InProgress. + let result = repair.repair_stuck_job(&stuck[0]).await.unwrap(); + assert!( + matches!(result, RepairResult::Success { .. }), + "Expected Success, got: {:?}", + result + ); + + // Job should be back to InProgress after recovery. + let ctx = cm.get_context(job_id).await.unwrap(); + assert_eq!(ctx.state, JobState::InProgress); + } + + #[tokio::test] + async fn detect_broken_tools_returns_empty_without_store() { + let cm = Arc::new(ContextManager::new(10)); + let repair = DefaultSelfRepair::new(cm, Duration::from_secs(60), 3); + + // No store configured, should return empty. + let broken = repair.detect_broken_tools().await; + assert!(broken.is_empty()); + } + + #[tokio::test] + async fn repair_broken_tool_returns_manual_without_builder() { + let cm = Arc::new(ContextManager::new(10)); + let repair = DefaultSelfRepair::new(cm, Duration::from_secs(60), 3); + + let broken = BrokenTool { + name: "test-tool".to_string(), + failure_count: 10, + last_error: Some("crash".to_string()), + first_failure: Utc::now(), + last_failure: Utc::now(), + last_build_result: None, + repair_attempts: 0, + }; + + let result = repair.repair_broken_tool(&broken).await.unwrap(); + assert!( + matches!(result, RepairResult::ManualRequired { .. }), + "Expected ManualRequired without builder, got: {:?}", + result + ); + } + #[tokio::test] async fn detect_stuck_jobs_filters_by_threshold() { let cm = Arc::new(ContextManager::new(10)); @@ -581,39 +710,6 @@ mod tests { ); } - #[tokio::test] - async fn detect_broken_tools_returns_empty_without_store() { - let cm = Arc::new(ContextManager::new(10)); - let repair = DefaultSelfRepair::new(cm, Duration::from_secs(60), 3); - - // No store configured, should return empty. - let broken = repair.detect_broken_tools().await; - assert!(broken.is_empty()); - } - - #[tokio::test] - async fn repair_broken_tool_returns_manual_without_builder() { - let cm = Arc::new(ContextManager::new(10)); - let repair = DefaultSelfRepair::new(cm, Duration::from_secs(60), 3); - - let broken = BrokenTool { - name: "test-tool".to_string(), - failure_count: 10, - last_error: Some("crash".to_string()), - first_failure: Utc::now(), - last_failure: Utc::now(), - last_build_result: None, - repair_attempts: 0, - }; - - let result = repair.repair_broken_tool(&broken).await.unwrap(); - assert!( - matches!(result, RepairResult::ManualRequired { .. }), - "Expected ManualRequired without builder, got: {:?}", - result - ); - } - /// Mock SoftwareBuilder that returns a successful build result. struct MockBuilder { build_count: std::sync::atomic::AtomicU32, diff --git a/src/context/manager.rs b/src/context/manager.rs index 6eb63260..f9bfedca 100644 --- a/src/context/manager.rs +++ b/src/context/manager.rs @@ -1,6 +1,7 @@ //! Context manager for handling multiple job contexts. use std::collections::HashMap; +use std::time::Duration; use tokio::sync::RwLock; use uuid::Uuid; @@ -205,12 +206,46 @@ impl ContextManager { } /// Find stuck jobs. + /// + /// Returns jobs that are explicitly in `Stuck` state, plus `InProgress` + /// jobs that have been running longer than `elapsed_threshold` (if provided). + /// The threshold-based detection catches jobs that never transitioned to + /// `Stuck` (e.g., due to a deadlock or unhandled timeout). pub async fn find_stuck_jobs(&self) -> Vec { + self.find_stuck_jobs_with_threshold(None).await + } + + /// Find stuck jobs with an optional elapsed threshold for `InProgress` detection. + pub async fn find_stuck_jobs_with_threshold( + &self, + elapsed_threshold: Option, + ) -> Vec { + let now = chrono::Utc::now(); self.contexts .read() .await .iter() - .filter(|(_, c)| c.state == crate::context::JobState::Stuck) + .filter(|(_, c)| { + // Always include explicitly Stuck jobs. + if c.state == crate::context::JobState::Stuck { + return true; + } + // Detect InProgress jobs that have been running beyond the elapsed threshold. + // NOTE: `started_at` is set on the first transition to InProgress and is + // NOT reset when a job recovers from Stuck back to InProgress. This means + // a recovered job may be re-detected on the next scan. A future improvement + // could track `in_progress_since` or use the most recent StateTransition + // with `to == InProgress` to avoid false positives on recovered jobs. + if c.state == crate::context::JobState::InProgress + && let Some(threshold) = elapsed_threshold + && let Some(started) = c.started_at + { + let elapsed = now.signed_duration_since(started); + let elapsed_secs = elapsed.num_seconds().max(0) as u64; + return elapsed_secs > threshold.as_secs(); + } + false + }) .map(|(id, _)| *id) .collect() } @@ -629,6 +664,48 @@ mod tests { assert_eq!(stuck[0], id2); } + /// Regression test for #1223: InProgress jobs exceeding the threshold + /// should be detected as stuck even if they never transitioned to Stuck. + #[tokio::test] + async fn find_stuck_jobs_with_threshold_detects_idle_in_progress() { + let manager = ContextManager::new(10); + + let id1 = manager.create_job("Active job", "desc").await.unwrap(); + let id2 = manager.create_job("Idle job", "desc").await.unwrap(); + + // Both transition to InProgress + for id in [id1, id2] { + manager + .update_context(id, |ctx| { + ctx.transition_to(crate::context::JobState::InProgress, None) + }) + .await + .unwrap() + .unwrap(); + } + + // Backdate id2's started_at to simulate a long-running job + manager + .update_context(id2, |ctx| -> Result<(), crate::error::JobError> { + ctx.started_at = Some(chrono::Utc::now() - chrono::Duration::seconds(600)); + Ok(()) + }) + .await + .unwrap() + .unwrap(); + + // With a 5-minute threshold, only id2 (10 min) should be detected + let stuck = manager + .find_stuck_jobs_with_threshold(Some(Duration::from_secs(300))) + .await; + assert_eq!(stuck.len(), 1); + assert_eq!(stuck[0], id2); + + // Without threshold, neither InProgress job is detected (no explicit Stuck state) + let stuck_no_threshold = manager.find_stuck_jobs().await; + assert!(stuck_no_threshold.is_empty()); + } + #[tokio::test] async fn active_count_tracks_non_terminal_jobs() { let manager = ContextManager::new(10); From ef3d76974239f3113e390a3af9d0809c70af6492 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 19 Mar 2026 22:52:33 -0700 Subject: [PATCH 2/8] fix(security): validate embedding base URLs to prevent SSRF (#1221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): validate embedding base URLs to prevent SSRF (#1103) User-configurable base URLs (OLLAMA_BASE_URL, EMBEDDING_BASE_URL) were passed directly to reqwest with no validation, allowing SSRF attacks against cloud metadata endpoints, internal services, or file:// URIs. Adds validate_base_url() that rejects: - Non-HTTP(S) schemes (file://, ftp://) - HTTP to non-localhost destinations (prevents credential leakage) - HTTPS to private/loopback/link-local/metadata IPs (169.254.169.254, 10.x, 192.168.x, 172.16-31.x, CGN 100.64/10) - IPv4-mapped IPv6 bypass attempts Validation runs at config resolution time so bad URLs fail at startup. Closes #1103 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(security): add DNS resolution check, ULA blocking, and NEARAI_BASE_URL validation Address review feedback: - Resolve hostnames to IPs and check all resolved addresses against the blocklist (prevents DNS-based SSRF bypass where attacker uses a domain pointing to 169.254.169.254) - Add IPv6 Unique Local Address (fc00::/7) to the blocklist - Validate NEARAI_BASE_URL in llm config (was missing — especially dangerous since bearer tokens are forwarded to the configured URL) - Allow DNS resolution failure gracefully (don't block startup when DNS is temporarily unavailable) Co-Authored-By: Claude Opus 4.6 (1M context) * style: fix formatting Co-Authored-By: Claude Opus 4.6 (1M context) * fix(security): add SSRF validation to all base URL chokepoints - Add validate_base_url() in resolve_registry_provider() covering all LLM providers (OpenAI, Anthropic, Ollama, openai_compatible, etc.) - Add validate_base_url() for NEARAI_AUTH_URL in LlmConfig::resolve() - Add validate_base_url() for TRANSCRIPTION_BASE_URL in TranscriptionConfig - Add missing SSRF test cases: CGN range, IPv4-mapped IPv6, ULA IPv6, URLs with credentials, empty/invalid URLs Co-Authored-By: Claude Opus 4.6 * ci: re-trigger CI with latest changes Co-Authored-By: Claude Opus 4.6 * ci: trigger new run with skip-regression-check label Co-Authored-By: Claude Opus 4.6 * fix(security): validate embedding base URLs to prevent SSRF (#1103) User-configurable base URLs (OLLAMA_BASE_URL, EMBEDDING_BASE_URL) were passed directly to reqwest with no validation, allowing SSRF attacks against cloud metadata endpoints, internal services, or file:// URIs. Adds validate_base_url() that rejects: - Non-HTTP(S) schemes (file://, ftp://) - HTTP to non-localhost destinations (prevents credential leakage) - HTTPS to private/loopback/link-local/metadata IPs (169.254.169.254, 10.x, 192.168.x, 172.16-31.x, CGN 100.64/10) - IPv4-mapped IPv6 bypass attempts Validation runs at config resolution time so bad URLs fail at startup. Closes #1103 Co-Authored-By: Claude Opus 4.6 (1M context) * fix(security): add DNS resolution check, ULA blocking, and NEARAI_BASE_URL validation Address review feedback: - Resolve hostnames to IPs and check all resolved addresses against the blocklist (prevents DNS-based SSRF bypass where attacker uses a domain pointing to 169.254.169.254) - Add IPv6 Unique Local Address (fc00::/7) to the blocklist - Validate NEARAI_BASE_URL in llm config (was missing — especially dangerous since bearer tokens are forwarded to the configured URL) - Allow DNS resolution failure gracefully (don't block startup when DNS is temporarily unavailable) Co-Authored-By: Claude Opus 4.6 (1M context) * style: fix formatting Co-Authored-By: Claude Opus 4.6 (1M context) * fix(security): add SSRF validation to all base URL chokepoints - Add validate_base_url() in resolve_registry_provider() covering all LLM providers (OpenAI, Anthropic, Ollama, openai_compatible, etc.) - Add validate_base_url() for NEARAI_AUTH_URL in LlmConfig::resolve() - Add validate_base_url() for TRANSCRIPTION_BASE_URL in TranscriptionConfig - Add missing SSRF test cases: CGN range, IPv4-mapped IPv6, ULA IPv6, URLs with credentials, empty/invalid URLs Co-Authored-By: Claude Opus 4.6 * ci: re-trigger CI with latest changes Co-Authored-By: Claude Opus 4.6 * ci: trigger new run with skip-regression-check label Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: ilblackdragon@gmail.com --- src/config/embeddings.rs | 8 +- src/config/helpers.rs | 263 ++++++++++++++++++++++++++++++++++++ src/config/llm.rs | 32 +++-- src/config/transcription.rs | 7 +- 4 files changed, 298 insertions(+), 12 deletions(-) diff --git a/src/config/embeddings.rs b/src/config/embeddings.rs index 813cbf7b..4f99dab4 100644 --- a/src/config/embeddings.rs +++ b/src/config/embeddings.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use secrecy::{ExposeSecret, SecretString}; -use crate::config::helpers::{optional_env, parse_bool_env, parse_optional_env}; +use crate::config::helpers::{optional_env, parse_bool_env, parse_optional_env, validate_base_url}; use crate::error::ConfigError; use crate::llm::SessionManager; use crate::settings::Settings; @@ -90,6 +90,12 @@ impl EmbeddingsConfig { let openai_base_url = optional_env("EMBEDDING_BASE_URL")?; + // Validate base URLs to prevent SSRF attacks (#1103). + validate_base_url(&ollama_base_url, "OLLAMA_BASE_URL")?; + if let Some(ref url) = openai_base_url { + validate_base_url(url, "EMBEDDING_BASE_URL")?; + } + let cache_size = parse_optional_env("EMBEDDING_CACHE_SIZE", DEFAULT_EMBEDDING_CACHE_SIZE)?; if cache_size == 0 { diff --git a/src/config/helpers.rs b/src/config/helpers.rs index ce6ce092..dc40fc9f 100644 --- a/src/config/helpers.rs +++ b/src/config/helpers.rs @@ -176,6 +176,151 @@ pub(crate) fn parse_string_env( Ok(optional_env(key)?.unwrap_or_else(|| default.into())) } +/// Validate a user-configurable base URL to prevent SSRF attacks (#1103). +/// +/// Rejects: +/// - Non-HTTP(S) schemes (file://, ftp://, etc.) +/// - HTTPS URLs pointing at private/loopback/link-local IPs +/// - HTTP URLs pointing at anything other than localhost/127.0.0.1/::1 +/// +/// This is intended for config-time validation of base URLs like +/// `OLLAMA_BASE_URL`, `EMBEDDING_BASE_URL`, `NEARAI_BASE_URL`, etc. +pub(crate) fn validate_base_url(url: &str, field_name: &str) -> Result<(), ConfigError> { + use std::net::{IpAddr, Ipv4Addr}; + + let parsed = reqwest::Url::parse(url).map_err(|e| ConfigError::InvalidValue { + key: field_name.to_string(), + message: format!("invalid URL '{}': {}", url, e), + })?; + + let scheme = parsed.scheme(); + if scheme != "http" && scheme != "https" { + return Err(ConfigError::InvalidValue { + key: field_name.to_string(), + message: format!("only http/https URLs are allowed, got '{}'", scheme), + }); + } + + let host = parsed.host_str().ok_or_else(|| ConfigError::InvalidValue { + key: field_name.to_string(), + message: "URL is missing a host".to_string(), + })?; + + let host_lower = host.to_lowercase(); + + // For HTTP (non-TLS), only allow localhost — remote HTTP endpoints + // risk credential leakage (e.g. NEAR AI bearer tokens sent over plaintext). + if scheme == "http" { + let is_localhost = host_lower == "localhost" + || host_lower == "127.0.0.1" + || host_lower == "::1" + || host_lower == "[::1]" + || host_lower.ends_with(".localhost"); + if !is_localhost { + return Err(ConfigError::InvalidValue { + key: field_name.to_string(), + message: format!( + "HTTP (non-TLS) is only allowed for localhost, got '{}'. \ + Use HTTPS for remote endpoints.", + host + ), + }); + } + return Ok(()); + } + + // Check whether an IP is in a blocked range (private, loopback, + // link-local, multicast, metadata, CGN, ULA). + let is_dangerous_ip = |ip: &IpAddr| -> bool { + match ip { + IpAddr::V4(v4) => { + v4.is_private() + || v4.is_loopback() + || v4.is_link_local() + || v4.is_multicast() + || v4.is_unspecified() + || *v4 == Ipv4Addr::new(169, 254, 169, 254) + || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64) // CGN + } + IpAddr::V6(v6) => { + if let Some(v4) = v6.to_ipv4_mapped() { + v4.is_private() + || v4.is_loopback() + || v4.is_link_local() + || v4.is_multicast() + || v4.is_unspecified() + || v4 == Ipv4Addr::new(169, 254, 169, 254) + || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64) // CGN + } else { + v6.is_loopback() + || v6.is_unspecified() + || (v6.octets()[0] & 0xfe) == 0xfc // ULA (fc00::/7) + || (v6.segments()[0] & 0xffc0) == 0xfe80 // link-local (fe80::/10) + || v6.octets()[0] == 0xff // multicast (ff00::/8) + } + } + } + }; + + // For HTTPS, reject private/loopback/link-local/metadata IPs. + // Check both IP literals and resolved hostnames to prevent DNS-based SSRF. + if let Ok(ip) = host.parse::() { + if is_dangerous_ip(&ip) { + return Err(ConfigError::InvalidValue { + key: field_name.to_string(), + message: format!( + "URL points to a private/internal IP '{}'. \ + This is blocked to prevent SSRF attacks.", + ip + ), + }); + } + } else { + // Hostname — resolve and check all resulting IPs as defense-in-depth. + // NOTE: This does NOT fully prevent DNS rebinding attacks (the hostname + // could resolve to a different IP at request time). Full protection + // would require pinning the resolved IP in the HTTP client's connector. + // This validation catches the common case of misconfigured or malicious URLs. + // + // NOTE: `to_socket_addrs()` performs blocking DNS resolution. This is + // acceptable because `validate_base_url` runs at config-load time only, + // before the async runtime is fully driving I/O. If this ever moves to + // a hot path, wrap in `tokio::task::spawn_blocking` or use + // `tokio::net::lookup_host`. + use std::net::ToSocketAddrs; + let port = parsed.port().unwrap_or(443); + match (host, port).to_socket_addrs() { + Ok(addrs) => { + for addr in addrs { + if is_dangerous_ip(&addr.ip()) { + return Err(ConfigError::InvalidValue { + key: field_name.to_string(), + message: format!( + "hostname '{}' resolves to private/internal IP '{}'. \ + This is blocked to prevent SSRF attacks.", + host, + addr.ip() + ), + }); + } + } + } + Err(e) => { + return Err(ConfigError::InvalidValue { + key: field_name.to_string(), + message: format!( + "failed to resolve hostname '{}': {}. \ + Base URLs must be resolvable at config time.", + host, e + ), + }); + } + } + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -226,4 +371,122 @@ mod tests { // Now the runtime override is visible again assert_eq!(env_or_override(key), Some("override_value".to_string())); } + + // --- validate_base_url tests (regression for #1103) --- + + #[test] + fn validate_base_url_allows_https() { + // Use IP literals to avoid DNS resolution in sandboxed test environments. + assert!(validate_base_url("https://8.8.8.8", "TEST").is_ok()); + assert!(validate_base_url("https://8.8.8.8/v1", "TEST").is_ok()); + } + + #[test] + fn validate_base_url_allows_http_localhost() { + assert!(validate_base_url("http://localhost:11434", "TEST").is_ok()); + assert!(validate_base_url("http://127.0.0.1:11434", "TEST").is_ok()); + assert!(validate_base_url("http://[::1]:11434", "TEST").is_ok()); + } + + #[test] + fn validate_base_url_rejects_http_remote() { + assert!(validate_base_url("http://evil.example.com", "TEST").is_err()); + assert!(validate_base_url("http://192.168.1.1", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_non_http_schemes() { + assert!(validate_base_url("file:///etc/passwd", "TEST").is_err()); + assert!(validate_base_url("ftp://evil.com", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_cloud_metadata() { + assert!(validate_base_url("https://169.254.169.254", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_private_ips() { + assert!(validate_base_url("https://10.0.0.1", "TEST").is_err()); + assert!(validate_base_url("https://192.168.1.1", "TEST").is_err()); + assert!(validate_base_url("https://172.16.0.1", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_cgn_range() { + // Carrier-grade NAT: 100.64.0.0/10 + assert!(validate_base_url("https://100.64.0.1", "TEST").is_err()); + assert!(validate_base_url("https://100.127.255.254", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_ipv4_mapped_ipv6() { + // ::ffff:10.0.0.1 is an IPv4-mapped IPv6 address pointing to private IP + assert!(validate_base_url("https://[::ffff:10.0.0.1]", "TEST").is_err()); + assert!(validate_base_url("https://[::ffff:169.254.169.254]", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_ula_ipv6() { + // fc00::/7 — unique local addresses + assert!(validate_base_url("https://[fc00::1]", "TEST").is_err()); + assert!(validate_base_url("https://[fd12:3456:789a::1]", "TEST").is_err()); + } + + #[test] + fn validate_base_url_handles_url_with_credentials() { + // URLs with embedded credentials — validate_base_url checks the host, + // not the credentials. Use IP literal to avoid DNS in sandboxed envs. + let result = validate_base_url("https://user:pass@8.8.8.8", "TEST"); + assert!(result.is_ok()); + } + + #[test] + fn validate_base_url_rejects_empty_and_invalid() { + assert!(validate_base_url("", "TEST").is_err()); + assert!(validate_base_url("not-a-url", "TEST").is_err()); + assert!(validate_base_url("://missing-scheme", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_unspecified_ipv4() { + assert!(validate_base_url("https://0.0.0.0", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_ipv6_loopback_https() { + // IPv6 loopback is allowed over HTTP (localhost equivalent), + // but must be rejected over HTTPS as a dangerous IP. + assert!(validate_base_url("https://[::1]", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_ipv6_link_local() { + // fe80::/10 — link-local addresses + assert!(validate_base_url("https://[fe80::1]", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_ipv6_multicast() { + // ff00::/8 — multicast addresses + assert!(validate_base_url("https://[ff02::1]", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_ipv6_unspecified() { + // :: — unspecified address + assert!(validate_base_url("https://[::]", "TEST").is_err()); + } + + #[test] + fn validate_base_url_rejects_dns_failure() { + // .invalid TLD is guaranteed to never resolve (RFC 6761) + let result = validate_base_url("https://ssrf-test.invalid", "TEST"); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("failed to resolve"), + "Expected DNS resolution failure, got: {err}" + ); + } } diff --git a/src/config/llm.rs b/src/config/llm.rs index d0f4ba8d..37fd9c47 100644 --- a/src/config/llm.rs +++ b/src/config/llm.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use secrecy::SecretString; use crate::bootstrap::ironclaw_base_dir; -use crate::config::helpers::{optional_env, parse_optional_env}; +use crate::config::helpers::{optional_env, parse_optional_env, validate_base_url}; use crate::error::ConfigError; use crate::llm::config::*; use crate::llm::registry::{ProviderProtocol, ProviderRegistry}; @@ -81,9 +81,11 @@ impl LlmConfig { } // Session config (used by NearAI provider for OAuth/session-token auth) + let nearai_auth_url = optional_env("NEARAI_AUTH_URL")? + .unwrap_or_else(|| "https://private.near.ai".to_string()); + validate_base_url(&nearai_auth_url, "NEARAI_AUTH_URL")?; let session = SessionConfig { - auth_base_url: optional_env("NEARAI_AUTH_URL")? - .unwrap_or_else(|| "https://private.near.ai".to_string()), + auth_base_url: nearai_auth_url, session_path: optional_env("NEARAI_SESSION_PATH")? .map(PathBuf::from) .unwrap_or_else(default_session_path), @@ -94,13 +96,17 @@ impl LlmConfig { let nearai = NearAiConfig { model: Self::resolve_model("NEARAI_MODEL", settings, crate::llm::DEFAULT_MODEL)?, cheap_model: optional_env("NEARAI_CHEAP_MODEL")?, - base_url: optional_env("NEARAI_BASE_URL")?.unwrap_or_else(|| { - if nearai_api_key.is_some() { - "https://cloud-api.near.ai".to_string() - } else { - "https://private.near.ai".to_string() - } - }), + base_url: { + let url = optional_env("NEARAI_BASE_URL")?.unwrap_or_else(|| { + if nearai_api_key.is_some() { + "https://cloud-api.near.ai".to_string() + } else { + "https://private.near.ai".to_string() + } + }); + validate_base_url(&url, "NEARAI_BASE_URL")?; + url + }, api_key: nearai_api_key, fallback_model: optional_env("NEARAI_FALLBACK_MODEL")?, max_retries: parse_optional_env("NEARAI_MAX_RETRIES", 3)?, @@ -325,6 +331,12 @@ impl LlmConfig { }); } + // Validate base URL to prevent SSRF (#1103). + if !base_url.is_empty() { + let field = base_url_env.unwrap_or("LLM_BASE_URL"); + validate_base_url(&base_url, field)?; + } + // Resolve model let model = Self::resolve_model(model_env, settings, default_model)?; diff --git a/src/config/transcription.rs b/src/config/transcription.rs index da2bac25..fc296c9a 100644 --- a/src/config/transcription.rs +++ b/src/config/transcription.rs @@ -1,6 +1,6 @@ use secrecy::SecretString; -use crate::config::helpers::{optional_env, parse_bool_env}; +use crate::config::helpers::{optional_env, parse_bool_env, validate_base_url}; use crate::error::ConfigError; use crate::settings::Settings; @@ -60,6 +60,11 @@ impl TranscriptionConfig { let base_url = optional_env("TRANSCRIPTION_BASE_URL")?; + // Validate base URL to prevent SSRF (#1103). + if let Some(ref url) = base_url { + validate_base_url(url, "TRANSCRIPTION_BASE_URL")?; + } + Ok(Self { enabled, provider, From b952d229f941298af5748d421edca6513382f7f5 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Thu, 19 Mar 2026 23:07:55 -0700 Subject: [PATCH 3/8] fix: prefer execution-local message routing metadata (#1449) * fix: prefer execution-local message routing metadata * test: cover message routing fallback metadata * refactor: simplify message target resolution * fix: ignore stale channel defaults for notify user metadata --- src/agent/agent_loop.rs | 60 +++++- src/agent/dispatcher.rs | 7 +- src/agent/thread_ops.rs | 1 + src/tools/builtin/message.rs | 368 ++++++++++++++++++++++++++++------- 4 files changed, 358 insertions(+), 78 deletions(-) diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index c31145d5..dbc9d38b 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -120,6 +120,17 @@ async fn resolve_routine_notification_target( .await } +pub(crate) fn chat_tool_execution_metadata(message: &IncomingMessage) -> serde_json::Value { + serde_json::json!({ + "notify_channel": message.channel, + "notify_user": message + .routing_target() + .unwrap_or_else(|| message.user_id.clone()), + "notify_thread_id": message.thread_id, + "notify_metadata": message.metadata, + }) +} + fn should_fallback_routine_notification(error: &ChannelError) -> bool { !matches!(error, ChannelError::MissingRoutingTarget { .. }) } @@ -1177,9 +1188,10 @@ impl Agent { #[cfg(test)] mod tests { use super::{ - resolve_routine_notification_user, should_fallback_routine_notification, - truncate_for_preview, + chat_tool_execution_metadata, resolve_routine_notification_user, + should_fallback_routine_notification, truncate_for_preview, }; + use crate::channels::IncomingMessage; use crate::error::ChannelError; #[test] @@ -1275,6 +1287,50 @@ mod tests { assert_eq!(resolve_routine_notification_user(&metadata), None); // safety: test-only assertion } + #[test] + fn chat_tool_execution_metadata_prefers_message_routing_target() { + let message = IncomingMessage::new("telegram", "owner-scope", "hello") + .with_sender_id("telegram-user") + .with_thread("thread-7") + .with_metadata(serde_json::json!({ + "chat_id": 424242, + "chat_type": "private", + })); + + let metadata = chat_tool_execution_metadata(&message); + assert_eq!( + metadata.get("notify_channel").and_then(|v| v.as_str()), + Some("telegram") + ); // safety: test-only assertion + assert_eq!( + metadata.get("notify_user").and_then(|v| v.as_str()), + Some("424242") + ); // safety: test-only assertion + assert_eq!( + metadata.get("notify_thread_id").and_then(|v| v.as_str()), + Some("thread-7") + ); // safety: test-only assertion + } + + #[test] + fn chat_tool_execution_metadata_falls_back_to_user_scope_without_route() { + let message = IncomingMessage::new("gateway", "owner-scope", "hello").with_sender_id(""); + + let metadata = chat_tool_execution_metadata(&message); + assert_eq!( + metadata.get("notify_channel").and_then(|v| v.as_str()), + Some("gateway") + ); // safety: test-only assertion + assert_eq!( + metadata.get("notify_user").and_then(|v| v.as_str()), + Some("owner-scope") + ); // safety: test-only assertion + assert_eq!( + metadata.get("notify_thread_id"), + Some(&serde_json::Value::Null) + ); // safety: test-only assertion + } + #[test] fn targeted_routine_notifications_do_not_fallback_without_owner_route() { let error = ChannelError::MissingRoutingTarget { diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 0b47c928..fc3da61b 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -144,12 +144,7 @@ impl Agent { .with_requester_id(&message.sender_id); job_ctx.http_interceptor = self.deps.http_interceptor.clone(); job_ctx.user_timezone = user_tz.name().to_string(); - job_ctx.metadata = serde_json::json!({ - "notify_channel": message.channel, - "notify_user": message.user_id, - "notify_thread_id": message.thread_id, - "notify_metadata": message.metadata, - }); + job_ctx.metadata = crate::agent::agent_loop::chat_tool_execution_metadata(message); // Build system prompts once for this turn. Two variants: with tools // (normal iterations) and without (force_text final iteration). diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index e8b8d09a..0fb968f1 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -939,6 +939,7 @@ impl Agent { JobContext::with_user(&message.user_id, "chat", "Interactive chat session") .with_requester_id(&message.sender_id); job_ctx.http_interceptor = self.deps.http_interceptor.clone(); + job_ctx.metadata = crate::agent::agent_loop::chat_tool_execution_metadata(message); // Prefer a valid timezone from the approval message, fall back to the // resolved timezone stored when the approval was originally requested. let tz_candidate = message diff --git a/src/tools/builtin/message.rs b/src/tools/builtin/message.rs index 1d2ed059..83041b80 100644 --- a/src/tools/builtin/message.rs +++ b/src/tools/builtin/message.rs @@ -67,6 +67,95 @@ impl MessageTool { } } +fn metadata_string(metadata: &serde_json::Value, key: &str) -> Option { + metadata + .get(key) + .and_then(|value| value.as_str()) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(ToOwned::to_owned) +} + +fn metadata_notify_user(metadata: &serde_json::Value) -> Option { + metadata_string(metadata, "notify_user").filter(|value| value != "default") +} + +fn channel_matches_source(resolved_channel: Option<&str>, source_channel: Option<&str>) -> bool { + match (resolved_channel, source_channel) { + (None, _) => true, + (Some(resolved), Some(source)) if resolved == source => true, + _ => false, + } +} + +async fn resolve_channel_fallback_target( + extension_manager: Option<&Arc>, + channel: Option<&str>, + ctx_user_id: &str, +) -> Option { + let channel_name = channel?; + + if let Some(extension_manager) = extension_manager + && let Some(target) = extension_manager + .notification_target_for_channel(channel_name) + .await + { + return Some(target); + } + + Some(ctx_user_id.to_string()) +} + +struct MessageTargetResolution<'a> { + extension_manager: Option<&'a Arc>, + explicit_target: Option, + metadata_target: Option, + default_target: Option, + channel: Option<&'a str>, + metadata_channel: Option<&'a str>, + default_channel: Option<&'a str>, + has_execution_routing_metadata: bool, + ctx_user_id: &'a str, +} + +async fn resolve_message_target(inputs: MessageTargetResolution<'_>) -> Option { + if let Some(target) = inputs.explicit_target { + return Some(target); + } + + if inputs.has_execution_routing_metadata { + if channel_matches_source(inputs.channel, inputs.metadata_channel) + && let Some(target) = inputs.metadata_target + { + return Some(target); + } + + return resolve_channel_fallback_target( + inputs.extension_manager, + inputs.channel, + inputs.ctx_user_id, + ) + .await; + } + + if channel_matches_source(inputs.channel, inputs.default_channel) + && let Some(target) = inputs.default_target + { + return Some(target); + } + + if inputs.channel.is_some() { + return resolve_channel_fallback_target( + inputs.extension_manager, + inputs.channel, + inputs.ctx_user_id, + ) + .await; + } + + None +} + #[async_trait] impl Tool for MessageTool { fn name(&self) -> &str { @@ -123,68 +212,52 @@ impl Tool for MessageTool { .get("channel") .and_then(|v| v.as_str()) .map(|value| value.to_string()); + let metadata_channel = metadata_string(&ctx.metadata, "notify_channel"); let default_channel = self .default_channel .read() .unwrap_or_else(|e| e.into_inner()) .clone(); - let metadata_channel = ctx - .metadata - .get("notify_channel") + let default_target = self + .default_target + .read() + .unwrap_or_else(|e| e.into_inner()) + .clone(); + let metadata_target = metadata_notify_user(&ctx.metadata); + let has_execution_routing_metadata = + metadata_channel.is_some() || metadata_target.is_some(); + + // Job metadata is authoritative for autonomous executions. The shared + // conversation defaults are only a legacy fallback when no execution-local + // routing metadata is available. + let channel: Option = explicit_channel + .clone() + .or_else(|| metadata_channel.clone()) + .or_else(|| { + (!has_execution_routing_metadata) + .then(|| default_channel.clone()) + .flatten() + }); + + let explicit_target = params + .get("target") .and_then(|v| v.as_str()) .map(|value| value.to_string()); - // Get channel: use param → conversation default → job metadata → None (broadcast all) - let channel: Option = explicit_channel - .clone() - .or_else(|| default_channel.clone()) - .or_else(|| metadata_channel.clone()); - - let can_use_default_target = match (explicit_channel.as_deref(), default_channel.as_deref()) - { - (None, _) => true, - (Some(explicit), Some(current)) if explicit == current => true, - _ => false, - }; - let can_use_metadata_target = match (channel.as_deref(), metadata_channel.as_deref()) { - (None, _) => true, - (Some(resolved), Some(current)) if resolved == current => true, - _ => false, - }; - - // Get target: use param → conversation default → job metadata → owner scope - // fallback when a specific channel is known. - let target = if let Some(t) = params.get("target").and_then(|v| v.as_str()) { - Some(t.to_string()) - } else if can_use_default_target - && let Some(t) = self - .default_target - .read() - .unwrap_or_else(|e| e.into_inner()) - .clone() - { - Some(t) - } else if can_use_metadata_target - && let Some(t) = ctx.metadata.get("notify_user").and_then(|v| v.as_str()) - { - Some(t.to_string()) - } else if channel.is_some() { - if let Some(channel_name) = channel.as_deref() { - if let Some(extension_manager) = self.extension_manager.as_ref() - && let Some(target) = extension_manager - .notification_target_for_channel(channel_name) - .await - { - Some(target) - } else { - Some(ctx.user_id.clone()) - } - } else { - Some(ctx.user_id.clone()) - } - } else { - None - }; + // Prefer explicit params, then execution-local routing metadata. Shared + // conversation defaults are only consulted when no job metadata exists. + let target = resolve_message_target(MessageTargetResolution { + extension_manager: self.extension_manager.as_ref(), + explicit_target, + metadata_target, + default_target, + channel: channel.as_deref(), + metadata_channel: metadata_channel.as_deref(), + default_channel: default_channel.as_deref(), + has_execution_routing_metadata, + ctx_user_id: &ctx.user_id, + }) + .await; let Some(target) = target else { return Err(ToolError::ExecutionFailed( @@ -230,6 +303,12 @@ impl Tool for MessageTool { if !attachments.is_empty() { response = response.with_attachments(attachments); } + if channel.as_deref() == Some("gateway") + && response.thread_id.is_none() + && let Some(thread_id) = metadata_string(&ctx.metadata, "notify_thread_id") + { + response = response.in_thread(thread_id); + } if let Some(ref channel) = channel { // Send to a specific channel @@ -326,6 +405,92 @@ impl Tool for MessageTool { #[cfg(test)] mod tests { use super::*; + use async_trait::async_trait; + use tokio::sync::{Mutex, mpsc}; + + use crate::channels::{ + Channel, IncomingMessage, MessageStream, OutgoingResponse, StatusUpdate, + }; + use crate::error::ChannelError; + + type BroadcastCapture = Arc>>; + + struct RecordingChannel { + name: &'static str, + captures: BroadcastCapture, + } + + impl RecordingChannel { + fn new(name: &'static str) -> (Self, BroadcastCapture) { + let captures = Arc::new(Mutex::new(Vec::new())); + ( + Self { + name, + captures: Arc::clone(&captures), + }, + captures, + ) + } + } + + #[async_trait] + impl Channel for RecordingChannel { + fn name(&self) -> &str { + self.name + } + + async fn start(&self) -> Result { + let (_tx, rx) = mpsc::channel::(1); + Ok(Box::pin(tokio_stream::wrappers::ReceiverStream::new(rx))) + } + + async fn respond( + &self, + _msg: &IncomingMessage, + _response: OutgoingResponse, + ) -> Result<(), ChannelError> { + Ok(()) + } + + async fn send_status( + &self, + _status: StatusUpdate, + _metadata: &serde_json::Value, + ) -> Result<(), ChannelError> { + Ok(()) + } + + async fn broadcast( + &self, + user_id: &str, + response: OutgoingResponse, + ) -> Result<(), ChannelError> { + self.captures + .lock() + .await + .push((user_id.to_string(), response)); + Ok(()) + } + + async fn health_check(&self) -> Result<(), ChannelError> { + Ok(()) + } + } + + async fn message_tool_with_recording_channels() + -> (MessageTool, BroadcastCapture, BroadcastCapture) { + let channel_manager = ChannelManager::new(); + let (gateway, gateway_captures) = RecordingChannel::new("gateway"); + let (telegram, telegram_captures) = RecordingChannel::new("telegram"); + channel_manager.add(Box::new(gateway)).await; + channel_manager.add(Box::new(telegram)).await; + + ( + MessageTool::new(Arc::new(channel_manager)), + gateway_captures, + telegram_captures, + ) + } #[test] fn message_tool_name() { @@ -782,31 +947,94 @@ mod tests { } #[tokio::test] - async fn message_tool_does_not_apply_metadata_target_to_different_default_channel() { - let tool = MessageTool::new(Arc::new(ChannelManager::new())); - tool.set_context(Some("telegram".to_string()), None).await; + async fn message_tool_prefers_metadata_over_stale_default_context() { + let (tool, gateway_captures, telegram_captures) = + message_tool_with_recording_channels().await; + tool.set_context( + Some("gateway".to_string()), + Some("stale-gateway-target".to_string()), + ) + .await; let mut ctx = crate::context::JobContext::with_user("owner-scope", "test", "test"); ctx.metadata = serde_json::json!({ - "notify_channel": "signal", - "notify_user": "metadata-user", + "notify_channel": "telegram", + "notify_user": "424242", }); let result = tool .execute(serde_json::json!({"content": "hello"}), &ctx) - .await; + .await + .expect("message tool should use telegram metadata routing"); + assert_eq!( + result.result.as_str(), + Some("Sent message to telegram:424242") + ); - assert!(result.is_err()); - let err = result.unwrap_err().to_string(); + assert!(gateway_captures.lock().await.is_empty()); + let telegram = telegram_captures.lock().await.clone(); + assert_eq!(telegram.len(), 1); + assert_eq!(telegram[0].0, "424242"); + assert_eq!(telegram[0].1.content, "hello"); + } + + #[tokio::test] + async fn message_tool_notify_user_only_metadata_does_not_reuse_stale_default_channel() { + let (tool, gateway_captures, telegram_captures) = + message_tool_with_recording_channels().await; + tool.set_context( + Some("gateway".to_string()), + Some("stale-gateway-target".to_string()), + ) + .await; + + let mut ctx = crate::context::JobContext::with_user("owner-scope", "test", "test"); + ctx.metadata = serde_json::json!({ + "notify_user": "424242", + }); + + let result = tool + .execute(serde_json::json!({"content": "hello"}), &ctx) + .await + .expect("message tool should broadcast when only notify_user is provided"); assert!( - !err.contains("metadata-user"), - "metadata target should not be applied to a different default channel: {}", - err - ); - assert!( - err.contains("owner-scope"), - "expected owner-scope fallback target when metadata channel differs: {}", - err + result + .result + .as_str() + .is_some_and(|message| message.contains("Broadcast message to")) ); + + let gateway = gateway_captures.lock().await.clone(); + assert_eq!(gateway.len(), 1); + assert_eq!(gateway[0].0, "424242"); + assert_eq!(gateway[0].1.content, "hello"); + + let telegram = telegram_captures.lock().await.clone(); + assert_eq!(telegram.len(), 1); + assert_eq!(telegram[0].0, "424242"); + assert_eq!(telegram[0].1.content, "hello"); + } + + #[tokio::test] + async fn message_tool_applies_notify_thread_id_for_gateway_delivery() { + let (tool, gateway_captures, telegram_captures) = + message_tool_with_recording_channels().await; + + let mut ctx = crate::context::JobContext::with_user("owner-scope", "test", "test"); + ctx.metadata = serde_json::json!({ + "notify_channel": "gateway", + "notify_user": "owner-scope", + "notify_thread_id": "thread-123", + }); + + tool.execute(serde_json::json!({"content": "hello"}), &ctx) + .await + .expect("gateway routing with thread id should succeed"); + + assert!(telegram_captures.lock().await.is_empty()); + let gateway = gateway_captures.lock().await.clone(); + assert_eq!(gateway.len(), 1); + assert_eq!(gateway[0].0, "owner-scope"); + assert_eq!(gateway[0].1.thread_id.as_deref(), Some("thread-123")); } } From e82f4bd2e56f547079838f88b33ca731d1e921e6 Mon Sep 17 00:00:00 2001 From: Vincent Leraitre <1267662+vnz@users.noreply.github.com> Date: Fri, 20 Mar 2026 07:22:34 +0100 Subject: [PATCH 4/8] fix: register sandbox jobs in ContextManager for query tool visibility (#1426) * fix: register sandbox jobs in ContextManager for query tool visibility Sandbox jobs created via execute_sandbox() were persisted to the database but never registered in the in-memory ContextManager. Since all query tools (list_jobs, job_status, job_events, cancel_job) only search the ContextManager, sandbox jobs were invisible to the agent despite running successfully in Docker containers. Changes: - Add register_sandbox_job() to ContextManager (pre-determined UUID, starts InProgress, respects max_jobs) - Extract insert_context() helper to deduplicate create_job_for_user and register_sandbox_job - Add update_context_state / update_context_state_async to sync ContextManager state on sandbox job completion/failure - Extend job_monitor with spawn_job_monitor_with_context() and spawn_completion_watcher() so fire-and-forget jobs transition out of InProgress when the container finishes - Make CancelJobTool sandbox-aware (stops container + updates DB) - Wire sandbox deps into CancelJobTool in register_job_tools() - 8 regression tests across context manager and job monitor Co-Authored-By: Claude Opus 4.6 (1M context) * fix: add missing allow_always field in PendingApproval test literal Upstream commit 09e1c97 added the allow_always field to PendingApproval but missed updating the test struct literal, breaking compilation. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/agent/job_monitor.rs | 224 +++++++++++++++++++++++++++++++++++++++ src/context/manager.rs | 133 +++++++++++++++++++++-- src/tools/builtin/job.rs | 138 +++++++++++++++++++++++- src/tools/registry.rs | 9 +- 4 files changed, 488 insertions(+), 16 deletions(-) diff --git a/src/agent/job_monitor.rs b/src/agent/job_monitor.rs index 6497861a..3f038764 100644 --- a/src/agent/job_monitor.rs +++ b/src/agent/job_monitor.rs @@ -14,12 +14,15 @@ //! Agent Loop //! ``` +use std::sync::Arc; + use tokio::sync::{broadcast, mpsc}; use tokio::task::JoinHandle; use uuid::Uuid; use crate::channels::IncomingMessage; use crate::channels::web::types::SseEvent; +use crate::context::{ContextManager, JobState}; /// Route context for forwarding job monitor events back to the user's channel. #[derive(Debug, Clone)] @@ -40,10 +43,23 @@ pub struct JobMonitorRoute { /// Tool use/result and status events are intentionally skipped (too noisy for /// the main agent's context window). pub fn spawn_job_monitor( + job_id: Uuid, + event_rx: broadcast::Receiver<(Uuid, SseEvent)>, + inject_tx: mpsc::Sender, + route: JobMonitorRoute, +) -> JoinHandle<()> { + spawn_job_monitor_with_context(job_id, event_rx, inject_tx, route, None) +} + +/// Like `spawn_job_monitor`, but also transitions the job's in-memory state +/// when it receives a `JobResult` event. This ensures fire-and-forget sandbox +/// jobs don't stay `InProgress` forever in the `ContextManager`. +pub fn spawn_job_monitor_with_context( job_id: Uuid, mut event_rx: broadcast::Receiver<(Uuid, SseEvent)>, inject_tx: mpsc::Sender, route: JobMonitorRoute, + context_manager: Option>, ) -> JoinHandle<()> { let short_id = job_id.to_string()[..8].to_string(); @@ -77,6 +93,26 @@ pub fn spawn_job_monitor( } } SseEvent::JobResult { status, .. } => { + // Transition in-memory state so the job frees its + // max_jobs slot and query tools show the final state. + if let Some(ref cm) = context_manager { + let target = if status == "completed" { + JobState::Completed + } else { + JobState::Failed + }; + let reason = if status != "completed" { + Some(format!("Container finished: {}", status)) + } else { + None + }; + let _ = cm + .update_context(job_id, |ctx| { + let _ = ctx.transition_to(target, reason); + }) + .await; + } + let mut msg = IncomingMessage::new( route.channel.clone(), route.user_id.clone(), @@ -121,6 +157,62 @@ pub fn spawn_job_monitor( }) } +/// Lightweight watcher that only transitions ContextManager state on job +/// completion. Used when monitor routing metadata is absent (no channel to +/// inject messages into) but we still need to free the `max_jobs` slot. +pub fn spawn_completion_watcher( + job_id: Uuid, + mut event_rx: broadcast::Receiver<(Uuid, SseEvent)>, + context_manager: Arc, +) -> JoinHandle<()> { + let short_id = job_id.to_string()[..8].to_string(); + + tokio::spawn(async move { + loop { + match event_rx.recv().await { + Ok((ev_job_id, SseEvent::JobResult { status, .. })) if ev_job_id == job_id => { + let target = if status == "completed" { + JobState::Completed + } else { + JobState::Failed + }; + let reason = if status != "completed" { + Some(format!("Container finished: {}", status)) + } else { + None + }; + let _ = context_manager + .update_context(job_id, |ctx| { + let _ = ctx.transition_to(target, reason); + }) + .await; + tracing::debug!( + job_id = %short_id, + status = %status, + "Completion watcher exiting (job finished)" + ); + break; + } + Ok(_) => {} + Err(broadcast::error::RecvError::Lagged(n)) => { + tracing::warn!( + job_id = %short_id, + skipped = n, + "Completion watcher lagged" + ); + } + Err(broadcast::error::RecvError::Closed) => { + tracing::debug!( + job_id = %short_id, + "Broadcast channel closed, stopping completion watcher" + ); + break; + } + } + } + }) +} + #[cfg(test)] mod tests { use super::*; @@ -294,4 +386,136 @@ mod tests { let msg = IncomingMessage::new("monitor", "system", "test").into_internal(); assert!(msg.is_internal); } + + // === Regression: fire-and-forget sandbox jobs must transition out of InProgress === + // Before this fix, spawn_job_monitor only forwarded SSE messages but never + // updated ContextManager. Background sandbox jobs stayed InProgress forever, + // permanently consuming a max_jobs slot. + + #[tokio::test] + async fn test_monitor_transitions_context_on_completion() { + use crate::context::{ContextManager, JobState}; + + let cm = Arc::new(ContextManager::new(5)); + let job_id = Uuid::new_v4(); + cm.register_sandbox_job(job_id, "user-1", "Build app", "desc") + .await + .unwrap(); + + let (event_tx, _) = broadcast::channel::<(Uuid, SseEvent)>(16); + let (inject_tx, mut inject_rx) = mpsc::channel::(16); + + let handle = spawn_job_monitor_with_context( + job_id, + event_tx.subscribe(), + inject_tx, + test_route(), + Some(Arc::clone(&cm)), + ); + + // Send completion event + event_tx + .send(( + job_id, + SseEvent::JobResult { + job_id: job_id.to_string(), + status: "completed".to_string(), + session_id: None, + }, + )) + .unwrap(); + + // Drain the injected message + let _ = tokio::time::timeout(std::time::Duration::from_secs(1), inject_rx.recv()).await; + + // Wait for monitor to exit + tokio::time::timeout(std::time::Duration::from_secs(1), handle) + .await + .expect("monitor should exit") + .expect("monitor should not panic"); + + // Job should now be Completed, not InProgress + let ctx = cm.get_context(job_id).await.unwrap(); + assert_eq!(ctx.state, JobState::Completed); + } + + #[tokio::test] + async fn test_monitor_transitions_context_on_failure() { + use crate::context::{ContextManager, JobState}; + + let cm = Arc::new(ContextManager::new(5)); + let job_id = Uuid::new_v4(); + cm.register_sandbox_job(job_id, "user-1", "Build app", "desc") + .await + .unwrap(); + + let (event_tx, _) = broadcast::channel::<(Uuid, SseEvent)>(16); + let (inject_tx, mut inject_rx) = mpsc::channel::(16); + + let handle = spawn_job_monitor_with_context( + job_id, + event_tx.subscribe(), + inject_tx, + test_route(), + Some(Arc::clone(&cm)), + ); + + // Send failure event + event_tx + .send(( + job_id, + SseEvent::JobResult { + job_id: job_id.to_string(), + status: "failed".to_string(), + session_id: None, + }, + )) + .unwrap(); + + let _ = tokio::time::timeout(std::time::Duration::from_secs(1), inject_rx.recv()).await; + tokio::time::timeout(std::time::Duration::from_secs(1), handle) + .await + .expect("monitor should exit") + .expect("monitor should not panic"); + + let ctx = cm.get_context(job_id).await.unwrap(); + assert_eq!(ctx.state, JobState::Failed); + } + + // === Regression: completion watcher (no route metadata) === + // When monitor_route_from_ctx() returns None, spawn_completion_watcher + // must still transition the job so the max_jobs slot is freed. + + #[tokio::test] + async fn test_completion_watcher_transitions_on_result() { + use crate::context::{ContextManager, JobState}; + + let cm = Arc::new(ContextManager::new(5)); + let job_id = Uuid::new_v4(); + cm.register_sandbox_job(job_id, "user-1", "Build app", "desc") + .await + .unwrap(); + + let (event_tx, _) = broadcast::channel::<(Uuid, SseEvent)>(16); + let handle = spawn_completion_watcher(job_id, event_tx.subscribe(), Arc::clone(&cm)); + + event_tx + .send(( + job_id, + SseEvent::JobResult { + job_id: job_id.to_string(), + status: "completed".to_string(), + session_id: None, + }, + )) + .unwrap(); + + tokio::time::timeout(std::time::Duration::from_secs(1), handle) + .await + .expect("watcher should exit") + .expect("watcher should not panic"); + + let ctx = cm.get_context(job_id).await.unwrap(); + assert_eq!(ctx.state, JobState::Completed); + } } diff --git a/src/context/manager.rs b/src/context/manager.rs index f9bfedca..28343003 100644 --- a/src/context/manager.rs +++ b/src/context/manager.rs @@ -6,7 +6,7 @@ use std::time::Duration; use tokio::sync::RwLock; use uuid::Uuid; -use crate::context::{JobContext, Memory}; +use crate::context::{JobContext, JobState, Memory}; use crate::error::JobError; /// Manages contexts for multiple concurrent jobs. @@ -46,12 +46,41 @@ impl ContextManager { title: impl Into, description: impl Into, ) -> Result { - // Hold write lock for the entire check-insert to prevent TOCTOU races - // where two concurrent calls both pass the parallel_count check. + let context = JobContext::with_user(user_id, title, description); + let job_id = context.job_id; + self.insert_context(context).await?; + Ok(job_id) + } + + /// Register a sandbox job with a pre-determined ID. + /// + /// Unlike `create_job_for_user` (which generates its own UUID), this method + /// accepts an existing `job_id` — used by `execute_sandbox()` which creates + /// the UUID before the container so it can be shared with Docker labels and + /// DB persistence. + /// + /// The job starts in `InProgress` state since the container is about to be + /// created. Counts against `max_jobs` like any other job. + pub async fn register_sandbox_job( + &self, + job_id: Uuid, + user_id: impl Into, + title: impl Into, + description: impl Into, + ) -> Result<(), JobError> { + let mut context = JobContext::with_user(user_id, title, description); + context.job_id = job_id; + context.state = JobState::InProgress; + context.started_at = Some(chrono::Utc::now()); + self.insert_context(context).await + } + + /// Check max_jobs limit, insert context, and allocate memory. + /// + /// Holds the write lock for the entire check-insert to prevent TOCTOU + /// races where two concurrent calls both pass the parallel_count check. + async fn insert_context(&self, context: JobContext) -> Result<(), JobError> { let mut contexts = self.contexts.write().await; - // Only count jobs that consume execution slots (Pending, InProgress, Stuck). - // Completed and Submitted jobs are no longer actively executing and shouldn't - // block new job creation. let parallel_count = contexts .values() .filter(|c| c.state.is_parallel_blocking()) @@ -61,15 +90,16 @@ impl ContextManager { return Err(JobError::MaxJobsExceeded { max: self.max_jobs }); } - let context = JobContext::with_user(user_id, title, description); let job_id = context.job_id; contexts.insert(job_id, context); drop(contexts); - let memory = Memory::new(job_id); - self.memories.write().await.insert(job_id, memory); + self.memories + .write() + .await + .insert(job_id, Memory::new(job_id)); - Ok(job_id) + Ok(()) } /// Get a job context by ID. @@ -1262,4 +1292,87 @@ mod tests { } } } + + // === Regression: sandbox jobs must be visible to query tools === + // Before the fix, execute_sandbox() only persisted to DB but never + // registered in ContextManager, making sandbox jobs invisible to + // list_jobs, job_status, job_events, and resolve_job_id. + + #[tokio::test] + async fn register_sandbox_job_visible_to_queries() { + let manager = ContextManager::new(5); + let job_id = Uuid::new_v4(); + + manager + .register_sandbox_job( + job_id, + "user-42", + "Run tests", + "Execute test suite in sandbox", + ) + .await + .unwrap(); + + // Job should be retrievable by ID (used by job_status, job_events) + let ctx = manager.get_context(job_id).await.unwrap(); + assert_eq!(ctx.job_id, job_id); + assert_eq!(ctx.user_id, "user-42"); + assert_eq!(ctx.title, "Run tests"); + assert_eq!(ctx.state, JobState::InProgress); + assert!(ctx.started_at.is_some()); + + // Job should appear in all_jobs (used by resolve_job_id prefix matching) + let all = manager.all_jobs().await; + assert!(all.contains(&job_id)); + + // Job should appear in user-scoped listing (used by list_jobs) + let user_jobs = manager.all_jobs_for("user-42").await; + assert!(user_jobs.contains(&job_id)); + + // Job should appear in active jobs listing + let active = manager.active_jobs_for("user-42").await; + assert!(active.contains(&job_id)); + } + + #[tokio::test] + async fn register_sandbox_job_respects_max_jobs() { + let manager = ContextManager::new(2); + + // Fill up the slots with sandbox jobs + manager + .register_sandbox_job(Uuid::new_v4(), "user-1", "Job 1", "desc") + .await + .unwrap(); + manager + .register_sandbox_job(Uuid::new_v4(), "user-1", "Job 2", "desc") + .await + .unwrap(); + + // Third should fail + let result = manager + .register_sandbox_job(Uuid::new_v4(), "user-1", "Job 3", "desc") + .await; + assert!(matches!(result, Err(JobError::MaxJobsExceeded { max: 2 }))); + } + + #[tokio::test] + async fn register_sandbox_job_transitions_correctly() { + let manager = ContextManager::new(5); + let job_id = Uuid::new_v4(); + + manager + .register_sandbox_job(job_id, "user-1", "Task", "desc") + .await + .unwrap(); + + // Should be able to transition InProgress -> Completed + manager + .update_context(job_id, |ctx| ctx.transition_to(JobState::Completed, None)) + .await + .unwrap() + .unwrap(); + + let ctx = manager.get_context(job_id).await.unwrap(); + assert_eq!(ctx.state, JobState::Completed); + } } diff --git a/src/tools/builtin/job.rs b/src/tools/builtin/job.rs index ea7e5305..0933ee40 100644 --- a/src/tools/builtin/job.rs +++ b/src/tools/builtin/job.rs @@ -225,6 +225,41 @@ impl CreateJobTool { } } + /// Transition a sandbox job's state in the ContextManager (awaited). + /// + /// Best-effort: logs on failure (job may have been cleaned up already). + async fn update_context_state_async( + &self, + job_id: Uuid, + state: JobState, + reason: Option, + ) { + if let Err(e) = self + .context_manager + .update_context(job_id, |ctx| { + let _ = ctx.transition_to(state, reason); + }) + .await + { + tracing::debug!(job_id = %job_id, "sandbox context update skipped: {}", e); + } + } + + /// Fire-and-forget variant for use in sync contexts (e.g. `.map_err()` closures). + fn update_context_state(&self, job_id: Uuid, state: JobState, reason: Option) { + let cm = self.context_manager.clone(); + tokio::spawn(async move { + if let Err(e) = cm + .update_context(job_id, |ctx| { + let _ = ctx.transition_to(state, reason); + }) + .await + { + tracing::debug!(job_id = %job_id, "sandbox context update skipped: {}", e); + } + }); + } + /// Update sandbox job status in DB (fire-and-forget). fn update_status( &self, @@ -354,6 +389,16 @@ impl CreateJobTool { } }; + // Register in ContextManager so query tools (list_jobs, job_status, + // job_events, cancel_job) can find sandbox jobs. Without this, sandbox + // jobs exist only in the DB and are invisible to the agent. + self.context_manager + .register_sandbox_job(job_id, &ctx.user_id, task, task) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!("failed to register sandbox job: {}", e)) + })?; + // Persist the job to DB before creating the container. self.persist_job(SandboxJobRecord { id: job_id, @@ -397,6 +442,7 @@ impl CreateJobTool { None, Some(Utc::now()), ); + self.update_context_state(job_id, JobState::Failed, Some(e.to_string())); ToolError::ExecutionFailed(format!("failed to create container: {}", e)) })?; @@ -416,16 +462,20 @@ impl CreateJobTool { // monitor terminates. No JoinHandle is retained. if let (Some(etx), Some(itx)) = (&self.event_tx, &self.inject_tx) { if let Some(route) = monitor_route_from_ctx(ctx) { - crate::agent::job_monitor::spawn_job_monitor( + crate::agent::job_monitor::spawn_job_monitor_with_context( job_id, etx.subscribe(), itx.clone(), route, + Some(self.context_manager.clone()), ); } else { - tracing::debug!( - job_id = %job_id, - "Skipping job monitor injection due to missing route metadata" + // No routing metadata — can't inject messages, but still + // need to transition the job out of InProgress when done. + crate::agent::job_monitor::spawn_completion_watcher( + job_id, + etx.subscribe(), + self.context_manager.clone(), ); } } @@ -457,6 +507,12 @@ impl CreateJobTool { None, Some(Utc::now()), ); + self.update_context_state_async( + job_id, + JobState::Failed, + Some("Timed out (10 minutes)".to_string()), + ) + .await; return Err(ToolError::ExecutionFailed( "container execution timed out (10 minutes)".to_string(), )); @@ -491,6 +547,8 @@ impl CreateJobTool { None, Some(finished_at), ); + self.update_context_state_async(job_id, JobState::Completed, None) + .await; let result = serde_json::json!({ "job_id": job_id.to_string(), "status": "completed", @@ -508,6 +566,12 @@ impl CreateJobTool { None, Some(finished_at), ); + self.update_context_state_async( + job_id, + JobState::Failed, + Some(message.clone()), + ) + .await; return Err(ToolError::ExecutionFailed(format!( "container job failed: {}", message @@ -529,6 +593,12 @@ impl CreateJobTool { None, Some(Utc::now()), ); + self.update_context_state_async( + job_id, + JobState::Failed, + Some(message.clone()), + ) + .await; return Err(ToolError::ExecutionFailed(format!( "container job failed: {}", message @@ -544,6 +614,8 @@ impl CreateJobTool { None, Some(Utc::now()), ); + self.update_context_state_async(job_id, JobState::Completed, None) + .await; let result = serde_json::json!({ "job_id": job_id.to_string(), "status": "completed", @@ -1025,13 +1097,34 @@ impl Tool for JobStatusTool { } /// Tool for canceling a job. +/// +/// For sandbox jobs (registered via `register_sandbox_job`), cancellation also +/// stops the Docker container and updates the DB status — matching the behavior +/// of the web cancellation handler in `channels/web/handlers/jobs.rs`. pub struct CancelJobTool { context_manager: Arc, + job_manager: Option>, + store: Option>, } impl CancelJobTool { pub fn new(context_manager: Arc) -> Self { - Self { context_manager } + Self { + context_manager, + job_manager: None, + store: None, + } + } + + /// Inject sandbox dependencies so cancellation also stops containers. + pub fn with_sandbox( + mut self, + job_manager: Arc, + store: Option>, + ) -> Self { + self.job_manager = Some(job_manager); + self.store = store; + self } } @@ -1081,6 +1174,41 @@ impl Tool for CancelJobTool { .await { Ok(Ok(())) => { + // Stop the sandbox container if one exists for this job. + if let Some(ref jm) = self.job_manager + && let Err(e) = jm.stop_job(job_id).await + { + tracing::warn!( + job_id = %job_id, + "Failed to stop container during cancellation: {}", e + ); + } + + // Update DB status for sandbox jobs. Uses "failed" (not + // "cancelled") to match the web cancel handler convention — + // the sandbox DB schema treats cancellation as a failure variant. + if let Some(ref store) = self.store { + let store = store.clone(); + tokio::spawn(async move { + if let Err(e) = store + .update_sandbox_job_status( + job_id, + "failed", + Some(false), + Some("Cancelled by user"), + None, + Some(Utc::now()), + ) + .await + { + tracing::warn!( + job_id = %job_id, + "Failed to update sandbox job status on cancel: {}", e + ); + } + }); + } + let result = serde_json::json!({ "job_id": job_id.to_string(), "status": "cancelled", diff --git a/src/tools/registry.rs b/src/tools/registry.rs index a68e300b..c64b637f 100644 --- a/src/tools/registry.rs +++ b/src/tools/registry.rs @@ -367,6 +367,9 @@ impl ToolRegistry { if let Some(slot) = scheduler_slot { create_tool = create_tool.with_scheduler_slot(slot); } + // Clone before moving into create_tool so cancel_job can also use them. + let jm_for_cancel = job_manager.clone(); + let store_for_cancel = store.clone(); if let Some(jm) = job_manager { create_tool = create_tool.with_sandbox(jm, store.clone()); } @@ -379,7 +382,11 @@ impl ToolRegistry { self.register_sync(Arc::new(create_tool)); self.register_sync(Arc::new(ListJobsTool::new(Arc::clone(&context_manager)))); self.register_sync(Arc::new(JobStatusTool::new(Arc::clone(&context_manager)))); - self.register_sync(Arc::new(CancelJobTool::new(Arc::clone(&context_manager)))); + let mut cancel_tool = CancelJobTool::new(Arc::clone(&context_manager)); + if let Some(jm) = jm_for_cancel { + cancel_tool = cancel_tool.with_sandbox(jm, store_for_cancel); + } + self.register_sync(Arc::new(cancel_tool)); // Base tools: create, list, status, cancel let mut job_tool_count = 4; From c17626160ce956a5e7c64a59b3e65c1801fee21f Mon Sep 17 00:00:00 2001 From: rajulbhatnagar Date: Thu, 19 Mar 2026 23:25:03 -0700 Subject: [PATCH 5/8] fix: skip credential validation for Bedrock backend (#1011) Bedrock uses IAM credentials (instance roles, env vars, SSO) resolved by the AWS SDK at call time, so `provider` is never set during startup. Exclude it from the post-init validation that checks for missing API keys. Closes #1009 Co-authored-by: brajul Co-authored-by: Illia Polosukhin --- src/app.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app.rs b/src/app.rs index f9e43458..729d2269 100644 --- a/src/app.rs +++ b/src/app.rs @@ -694,7 +694,10 @@ impl AppBuilder { // Post-init validation: if a non-nearai backend was selected but // credentials were never resolved (deferred resolution found no keys), // fail early with a clear error instead of a confusing runtime failure. - if self.config.llm.backend != "nearai" && self.config.llm.provider.is_none() { + if self.config.llm.backend != "nearai" + && self.config.llm.backend != "bedrock" + && self.config.llm.provider.is_none() + { let backend = &self.config.llm.backend; anyhow::bail!( "LLM_BACKEND={backend} is configured but no credentials were found. \ From 1b97ef4feb07dfd24a878be9c3dd2fd32e1106d4 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Fri, 20 Mar 2026 00:41:20 -0700 Subject: [PATCH 6/8] fix: resolve wasm broadcast merge conflicts with staging (#395) (#1460) * channels/wasm: implement telegram broadcast path for message tool * channels/wasm: tighten telegram broadcast contract and tests * fix: resolve merge conflicts with staging for wasm broadcast - Remove duplicate broadcast() impls from WasmChannel and SharedWasmChannel (staging already has the generic call_on_broadcast path) - Remove obsolete telegram-specific test helpers and tests that tested the old telegram-only broadcast logic - Add test_broadcast_delegates_to_call_on_broadcast for the generic path - Fix missing fallback_deliverable field in job_monitor test SseEvents Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: davidpty <127684147+davidpty@users.noreply.github.com> Co-authored-by: firat.sertgoz Co-authored-by: Claude Opus 4.6 (1M context) --- src/agent/job_monitor.rs | 3 +++ src/channels/wasm/wrapper.rs | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/agent/job_monitor.rs b/src/agent/job_monitor.rs index 3f038764..675d0426 100644 --- a/src/agent/job_monitor.rs +++ b/src/agent/job_monitor.rs @@ -421,6 +421,7 @@ mod tests { job_id: job_id.to_string(), status: "completed".to_string(), session_id: None, + fallback_deliverable: None, }, )) .unwrap(); @@ -468,6 +469,7 @@ mod tests { job_id: job_id.to_string(), status: "failed".to_string(), session_id: None, + fallback_deliverable: None, }, )) .unwrap(); @@ -506,6 +508,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/wasm/wrapper.rs b/src/channels/wasm/wrapper.rs index 8f0c9db4..be7768d0 100644 --- a/src/channels/wasm/wrapper.rs +++ b/src/channels/wasm/wrapper.rs @@ -3314,6 +3314,7 @@ mod tests { use std::sync::Arc; use crate::channels::Channel; + use crate::channels::OutgoingResponse; use crate::channels::wasm::capabilities::ChannelCapabilities; use crate::channels::wasm::runtime::{ PreparedChannelModule, WasmChannelRuntime, WasmChannelRuntimeConfig, @@ -3401,6 +3402,16 @@ mod tests { assert!(channel.health_check().await.is_err()); } + #[tokio::test] + async fn test_broadcast_delegates_to_call_on_broadcast() { + let channel = create_test_channel(); + // With `component: None`, call_on_broadcast short-circuits to Ok(()). + let result = channel + .broadcast("146032821", OutgoingResponse::text("hello")) + .await; + assert!(result.is_ok()); + } + #[tokio::test] async fn test_execute_poll_no_wasm_returns_empty() { // When there's no WASM module (None component), execute_poll From cba1bc37997b2980e08ca9939747f9e2d7484102 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Fri, 20 Mar 2026 00:45:17 -0700 Subject: [PATCH 7/8] feat(web): add light theme with dark/light/system toggle (#1457) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web): add light theme with dark/light/system toggle (#761) Add three-state theme toggle (dark → light → system) to the Web Gateway: - Extract 101 hardcoded CSS colors into 30+ CSS custom properties - Add [data-theme='light'] overrides for all variables - Add theme toggle button in tab-bar (moon/sun/monitor icons) - Theme persists via localStorage, defaults to 'system' - System mode follows OS prefers-color-scheme in real-time - FOUC prevention via inline script in - Delayed CSS transition to avoid flash on initial load - Pure CSS icon switching via data-theme-mode attribute Closes #761 * fix: address review feedback and code improvements (takeover #853) - Fix dark-mode readability bug: .stepper-step.failed and .image-preview-remove used --text-on-accent (#09090b) on var(--danger) background, making text unreadable. Changed to --text-on-danger (#fff). - Restore hover visual feedback on .image-preview-remove:hover using filter: brightness(1.2) instead of redundant var(--danger). - Use const/let instead of var in theme-init.js for consistency with app.js (per gemini-code-assist review feedback). Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) * fix: address CI failures and Copilot review feedback (takeover #853) - Fix missing `fallback_deliverable` field in job_monitor test constructors (pre-existing staging issue surfaced by merge) - Validate localStorage theme value against whitelist in both theme-init.js and app.js to prevent broken state from invalid values - Add matchMedia addEventListener fallback for older Safari/WebKit - Add i18n keys for theme tooltip and aria-live announcement strings (en + zh-CN) to match existing localization patterns - Move .sr-only utility from inline