diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 00488c70..5d4eabc0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,6 +12,7 @@ jobs: tests: name: Tests (${{ matrix.name }}) runs-on: ubuntu-latest + timeout-minutes: 45 strategy: fail-fast: false matrix: @@ -40,11 +41,14 @@ jobs: - name: Build WASM channels (for integration tests) run: ./scripts/build-wasm-extensions.sh --channels - name: Run Tests - run: cargo test ${{ matrix.flags }} -- --nocapture + run: | + timeout --signal=INT --kill-after=30s 40m \ + cargo test ${{ matrix.flags }} -- --nocapture heavy-integration-tests: name: Heavy Integration Tests runs-on: ubuntu-latest + timeout-minutes: 20 steps: - name: Checkout repository uses: actions/checkout@v6 @@ -58,9 +62,13 @@ jobs: - name: Build Telegram WASM channel run: cargo build --manifest-path channels-src/telegram/Cargo.toml --target wasm32-wasip2 --release - name: Run thread scheduling integration tests - run: cargo test --no-default-features --features libsql,integration --test e2e_thread_scheduling -- --nocapture + run: | + timeout --signal=INT --kill-after=30s 15m \ + cargo test --no-default-features --features libsql,integration --test e2e_thread_scheduling -- --nocapture - name: Run Telegram thread-scope regression test - run: cargo test --features integration --test telegram_auth_integration test_private_messages_use_chat_id_as_thread_scope -- --exact + run: | + timeout --signal=INT --kill-after=30s 10m \ + cargo test --features integration --test telegram_auth_integration test_private_messages_use_chat_id_as_thread_scope -- --exact telegram-tests: name: Telegram Channel Tests @@ -68,6 +76,7 @@ jobs: github.event_name != 'pull_request' || github.base_ref != 'staging' runs-on: ubuntu-latest + timeout-minutes: 15 steps: - name: Checkout repository uses: actions/checkout@v6 @@ -75,7 +84,9 @@ jobs: uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - name: Run Telegram Channel Tests - run: cargo test --manifest-path channels-src/telegram/Cargo.toml -- --nocapture + run: | + timeout --signal=INT --kill-after=30s 10m \ + cargo test --manifest-path channels-src/telegram/Cargo.toml -- --nocapture windows-build: name: Windows Build (${{ matrix.name }}) @@ -110,6 +121,7 @@ jobs: github.event_name != 'pull_request' || github.base_ref != 'staging' runs-on: ubuntu-latest + timeout-minutes: 30 steps: - name: Checkout repository uses: actions/checkout@v6 @@ -125,7 +137,9 @@ jobs: - name: Build all WASM extensions against current WIT run: ./scripts/build-wasm-extensions.sh - name: Instantiation test (host linker compatibility) - run: cargo test --all-features wit_compat -- --nocapture + run: | + timeout --signal=INT --kill-after=30s 20m \ + cargo test --all-features wit_compat -- --nocapture bench-compile: name: Benchmark Compilation diff --git a/Cargo.lock b/Cargo.lock index a144e43e..8cc63249 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1204,6 +1204,26 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "bytemuck" +version = "1.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c8efb64bd706a16a1bdde310ae86b351e4d21550d98d056f22f8a7f7a2183fec" +dependencies = [ + "bytemuck_derive", +] + +[[package]] +name = "bytemuck_derive" +version = "1.10.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f9abbd1bc6865053c427f7198e6af43bfdedc55ab791faed4fbd361d789575ff" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", +] + [[package]] name = "byteorder" version = "1.5.0" @@ -4319,9 +4339,10 @@ dependencies = [ [[package]] name = "monty" version = "0.0.8" -source = "git+https://github.com/pydantic/monty.git?branch=main#982709bd52b10a1cf3f244a67951523a0b016a7f" +source = "git+https://github.com/pydantic/monty.git?branch=main#4e1beaa7ebe04500f8873da3278538bfe0717070" dependencies = [ "ahash 0.8.12", + "bytemuck", "fancy-regex", "hashbrown 0.16.1", "indexmap 2.13.0", diff --git a/crates/ironclaw_common/src/event.rs b/crates/ironclaw_common/src/event.rs index 80cd570a..256aba3d 100644 --- a/crates/ironclaw_common/src/event.rs +++ b/crates/ironclaw_common/src/event.rs @@ -5,9 +5,35 @@ //! frames, but other subsystems (agent loop, orchestrator, extensions) //! produce and consume them too. -use serde::Serialize; +use serde::{Deserialize, Serialize}; -#[derive(Debug, Clone, Serialize)] +/// A single tool decision in a reasoning update (SSE DTO). +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ToolDecisionDto { + pub tool_name: String, + pub rationale: String, +} + +impl ToolDecisionDto { + /// Parse a list of tool decisions from a JSON array value. + pub fn from_json_array(value: &serde_json::Value) -> Vec { + value + .as_array() + .map(|arr| { + arr.iter() + .filter_map(|d| { + Some(Self { + tool_name: d.get("tool_name")?.as_str()?.to_string(), + rationale: d.get("rationale")?.as_str()?.to_string(), + }) + }) + .collect() + }) + .unwrap_or_default() + } +} + +#[derive(Debug, Clone, Serialize, Deserialize)] #[serde(tag = "type")] pub enum AppEvent { #[serde(rename = "response")] @@ -163,4 +189,205 @@ pub enum AppEvent { #[serde(skip_serializing_if = "Option::is_none")] message: Option, }, + + /// Agent reasoning update (why it chose specific tools). + #[serde(rename = "reasoning_update")] + ReasoningUpdate { + narrative: String, + decisions: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + thread_id: Option, + }, + + /// Reasoning update for a sandbox job. + #[serde(rename = "job_reasoning")] + JobReasoning { + job_id: String, + narrative: String, + decisions: Vec, + }, +} + +impl AppEvent { + /// The wire-format event type string (matches the `#[serde(rename)]` value). + pub fn event_type(&self) -> &'static str { + match self { + Self::Response { .. } => "response", + Self::Thinking { .. } => "thinking", + Self::ToolStarted { .. } => "tool_started", + Self::ToolCompleted { .. } => "tool_completed", + Self::ToolResult { .. } => "tool_result", + Self::StreamChunk { .. } => "stream_chunk", + Self::Status { .. } => "status", + Self::JobStarted { .. } => "job_started", + Self::ApprovalNeeded { .. } => "approval_needed", + Self::AuthRequired { .. } => "auth_required", + Self::AuthCompleted { .. } => "auth_completed", + Self::Error { .. } => "error", + Self::Heartbeat => "heartbeat", + Self::JobMessage { .. } => "job_message", + Self::JobToolUse { .. } => "job_tool_use", + Self::JobToolResult { .. } => "job_tool_result", + Self::JobStatus { .. } => "job_status", + Self::JobResult { .. } => "job_result", + Self::ImageGenerated { .. } => "image_generated", + Self::Suggestions { .. } => "suggestions", + Self::TurnCost { .. } => "turn_cost", + Self::ExtensionStatus { .. } => "extension_status", + Self::ReasoningUpdate { .. } => "reasoning_update", + Self::JobReasoning { .. } => "job_reasoning", + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Verify that `event_type()` returns the same string as the serde + /// `"type"` field for every variant. This catches drift between the + /// `#[serde(rename)]` attributes and the manual match arms. + #[test] + fn event_type_matches_serde_type_field() { + let variants: Vec = vec![ + AppEvent::Response { + content: String::new(), + thread_id: String::new(), + }, + AppEvent::Thinking { + message: String::new(), + thread_id: None, + }, + AppEvent::ToolStarted { + name: String::new(), + thread_id: None, + }, + AppEvent::ToolCompleted { + name: String::new(), + success: true, + error: None, + parameters: None, + thread_id: None, + }, + AppEvent::ToolResult { + name: String::new(), + preview: String::new(), + thread_id: None, + }, + AppEvent::StreamChunk { + content: String::new(), + thread_id: None, + }, + AppEvent::Status { + message: String::new(), + thread_id: None, + }, + AppEvent::JobStarted { + job_id: String::new(), + title: String::new(), + browse_url: String::new(), + }, + AppEvent::ApprovalNeeded { + request_id: String::new(), + tool_name: String::new(), + description: String::new(), + parameters: String::new(), + thread_id: None, + allow_always: false, + }, + AppEvent::AuthRequired { + extension_name: String::new(), + instructions: None, + auth_url: None, + setup_url: None, + }, + AppEvent::AuthCompleted { + extension_name: String::new(), + success: true, + message: String::new(), + }, + AppEvent::Error { + message: String::new(), + thread_id: None, + }, + AppEvent::Heartbeat, + AppEvent::JobMessage { + job_id: String::new(), + role: String::new(), + content: String::new(), + }, + AppEvent::JobToolUse { + job_id: String::new(), + tool_name: String::new(), + input: serde_json::Value::Null, + }, + AppEvent::JobToolResult { + job_id: String::new(), + tool_name: String::new(), + output: String::new(), + }, + AppEvent::JobStatus { + job_id: String::new(), + message: String::new(), + }, + AppEvent::JobResult { + job_id: String::new(), + status: String::new(), + session_id: None, + fallback_deliverable: None, + }, + AppEvent::ImageGenerated { + data_url: String::new(), + path: None, + thread_id: None, + }, + AppEvent::Suggestions { + suggestions: vec![], + thread_id: None, + }, + AppEvent::TurnCost { + input_tokens: 0, + output_tokens: 0, + cost_usd: String::new(), + thread_id: None, + }, + AppEvent::ExtensionStatus { + extension_name: String::new(), + status: String::new(), + message: None, + }, + AppEvent::ReasoningUpdate { + narrative: String::new(), + decisions: vec![], + thread_id: None, + }, + AppEvent::JobReasoning { + job_id: String::new(), + narrative: String::new(), + decisions: vec![], + }, + ]; + + for variant in &variants { + let json: serde_json::Value = serde_json::to_value(variant).unwrap(); + let serde_type = json["type"].as_str().unwrap(); + assert_eq!( + variant.event_type(), + serde_type, + "event_type() mismatch for variant: {:?}", + variant + ); + } + } + + #[test] + fn round_trip_deserialize() { + let original = AppEvent::Response { + content: "hello".to_string(), + thread_id: "t1".to_string(), + }; + let json = serde_json::to_string(&original).unwrap(); + let deserialized: AppEvent = serde_json::from_str(&json).unwrap(); + assert_eq!(deserialized.event_type(), "response"); + } } diff --git a/crates/ironclaw_common/src/lib.rs b/crates/ironclaw_common/src/lib.rs index 6822bad1..f52dc0aa 100644 --- a/crates/ironclaw_common/src/lib.rs +++ b/crates/ironclaw_common/src/lib.rs @@ -3,5 +3,5 @@ mod event; mod util; -pub use event::AppEvent; +pub use event::{AppEvent, ToolDecisionDto}; pub use util::truncate_preview; diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index 2b6d3970..b53fdf9e 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -16,6 +16,7 @@ use crate::agent::context_monitor::ContextMonitor; use crate::agent::heartbeat::spawn_heartbeat; use crate::agent::routine_engine::{RoutineEngine, spawn_cron_ticker}; use crate::agent::self_repair::{DefaultSelfRepair, RepairResult, SelfRepair}; +use crate::agent::session::ThreadState; use crate::agent::session_manager::SessionManager; use crate::agent::submission::{Submission, SubmissionParser, SubmissionResult}; use crate::agent::{HeartbeatConfig as AgentHeartbeatConfig, Router, Scheduler, SchedulerDeps}; @@ -84,6 +85,15 @@ fn resolve_owner_scope_notification_user( trimmed_option(explicit_user).or_else(|| trimmed_option(owner_fallback)) } +fn is_single_message_repl(message: &IncomingMessage) -> bool { + message.channel == "repl" + && message + .metadata + .get("single_message_mode") + .and_then(|value| value.as_bool()) + .unwrap_or(false) +} + async fn resolve_channel_notification_user( extension_manager: Option<&Arc>, channel: Option<&str>, @@ -1068,10 +1078,11 @@ impl Agent { } else { drop(sess); self.session_manager - .resolve_thread( + .resolve_thread_with_parsed_uuid( &message.user_id, &message.channel, message.conversation_scope(), + approval_thread_uuid, ) .await } @@ -1152,9 +1163,14 @@ impl Agent { && let Submission::UserInput { ref content } = submission && let Some(engine) = self.routine_engine().await { - let fired = engine - .check_event_triggers(&message.user_id, &message.channel, content) - .await; + let single_message_repl = is_single_message_repl(message); + // Use post-hook content so that BeforeInbound hooks that rewrite + // input are respected by event trigger matching. + let fired = if single_message_repl { + engine.check_event_triggers_and_wait(message, content).await + } else { + engine.check_event_triggers(message, content).await + }; if fired > 0 { tracing::debug!( channel = %message.channel, @@ -1162,10 +1178,16 @@ impl Agent { fired, "Consumed inbound user message with matching event-triggered routine(s)" ); - return Ok(Some(String::new())); + return if single_message_repl { + Ok(None) + } else { + Ok(Some(String::new())) + }; } } + let session_for_empty_exit = Arc::clone(&session); + // Process based on submission type let result = match submission { Submission::UserInput { content } => { @@ -1262,6 +1284,28 @@ impl Agent { command, message.channel ); + // /reasoning is special-cased here (not in handle_system_command) + // because it needs the session + thread_id to read turn reasoning + // data, which handle_system_command's signature doesn't provide. + if command == "reasoning" { + let result = self + .handle_reasoning_command(&args, &session, thread_id) + .await; + return match result { + SubmissionResult::Response { content } => Ok(Some(content)), + SubmissionResult::Ok { message } => Ok(message), + SubmissionResult::Error { message } => { + Ok(Some(format!("Error: {}", message))) + } + _ => { + if is_single_message_repl(message) { + Ok(None) + } else { + Ok(Some(String::new())) + } + } + }; + } // Authorization checks (including restart channel check) are enforced in handle_system_command self.handle_system_command(&command, &args, &message.channel) .await @@ -1321,7 +1365,26 @@ impl Agent { Ok(Some(content)) } } - SubmissionResult::Ok { message } => Ok(message), + SubmissionResult::Ok { + message: output_message, + } => { + let should_exit = + if output_message.as_deref() == Some("") && is_single_message_repl(message) { + let sess = session_for_empty_exit.lock().await; + sess.threads + .get(&thread_id) + .map(|thread| thread.state != ThreadState::AwaitingApproval) + .unwrap_or(true) + } else { + false + }; + + if should_exit { + Ok(None) + } else { + Ok(output_message) + } + } SubmissionResult::Error { message } => Ok(Some(format!("Error: {}", message))), SubmissionResult::Interrupted => Ok(Some("Interrupted.".into())), SubmissionResult::NeedApproval { .. } => { @@ -1337,7 +1400,7 @@ impl Agent { #[cfg(test)] mod tests { use super::{ - chat_tool_execution_metadata, resolve_routine_notification_user, + chat_tool_execution_metadata, is_single_message_repl, resolve_routine_notification_user, should_fallback_routine_notification, truncate_for_preview, }; use crate::channels::IncomingMessage; @@ -1499,4 +1562,17 @@ mod tests { assert!(should_fallback_routine_notification(&error)); // safety: test-only assertion } + + #[test] + fn single_message_repl_detection_requires_repl_channel_and_metadata_flag() { + let repl = IncomingMessage::new("repl", "owner-scope", "hello") + .with_metadata(serde_json::json!({ "single_message_mode": true })); + let gateway = IncomingMessage::new("gateway", "owner-scope", "hello") + .with_metadata(serde_json::json!({ "single_message_mode": true })); + let plain_repl = IncomingMessage::new("repl", "owner-scope", "hello"); + + assert!(is_single_message_repl(&repl)); // safety: test-only assertion + assert!(!is_single_message_repl(&gateway)); // safety: test-only assertion + assert!(!is_single_message_repl(&plain_repl)); // safety: test-only assertion + } } diff --git a/src/agent/agentic_loop.rs b/src/agent/agentic_loop.rs index cc6fd486..e61856dc 100644 --- a/src/agent/agentic_loop.rs +++ b/src/agent/agentic_loop.rs @@ -414,6 +414,7 @@ mod tests { id: "call_1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({}), + reasoning: None, }; let delegate = MockDelegate::new(vec![ tool_calls_output(vec![tool_call]), diff --git a/src/agent/commands.rs b/src/agent/commands.rs index b6aff3c0..e02b33db 100644 --- a/src/agent/commands.rs +++ b/src/agent/commands.rs @@ -465,6 +465,94 @@ impl Agent { } } + /// Handle `/reasoning [N|all]` — show reasoning history for the active thread. + pub(super) async fn handle_reasoning_command( + &self, + args: &[String], + session: &Arc>, + thread_id: Uuid, + ) -> SubmissionResult { + // Clone the turn data we need, then drop the session lock. + let turns_snapshot: Vec<( + usize, + Option, + Vec, + )>; + { + let sess = session.lock().await; + let thread = match sess.threads.get(&thread_id) { + Some(t) => t, + None => return SubmissionResult::error("No active thread."), + }; + + if thread.turns.is_empty() { + return SubmissionResult::ok_with_message("No turns yet."); + } + + // Parse argument: default=last turn, "all"=all turns, N=specific turn (1-based). + let selected: Vec<&crate::agent::session::Turn> = match args.first().map(|s| s.as_str()) + { + Some("all") => thread.turns.iter().collect(), + Some(n) => match n.parse::() { + Ok(0) => return SubmissionResult::error("Turn numbers start at 1."), + Ok(num) if num > thread.turns.len() => { + return SubmissionResult::error(format!( + "Turn {} does not exist (max: {}).", + num, + thread.turns.len() + )); + } + Ok(num) => vec![&thread.turns[num - 1]], + Err(_) => return SubmissionResult::error("Usage: /reasoning [N|all]"), + }, + None => { + // Default: last turn that has tool calls + match thread.turns.iter().rev().find(|t| !t.tool_calls.is_empty()) { + Some(t) => vec![t], + None => { + return SubmissionResult::ok_with_message("No turns with tool calls."); + } + } + } + }; + + turns_snapshot = selected + .into_iter() + .map(|t| (t.turn_number, t.narrative.clone(), t.tool_calls.clone())) + .collect(); + } + // Session lock is now dropped — format output without holding it. + + let mut output = String::new(); + for (turn_number, narrative, tool_calls) in &turns_snapshot { + output.push_str(&format!("--- Turn {} ---\n", turn_number + 1)); + if let Some(narrative) = narrative { + output.push_str(&format!("Reasoning: {}\n", narrative)); + } + if tool_calls.is_empty() { + output.push_str(" (no tool calls)\n"); + } else { + for tc in tool_calls { + let status = if tc.error.is_some() { + "error" + } else if tc.result.is_some() { + "ok" + } else { + "pending" + }; + output.push_str(&format!(" {} [{}]", tc.name, status)); + if let Some(ref rationale) = tc.rationale { + output.push_str(&format!(" — {}", rationale)); + } + output.push('\n'); + } + } + output.push('\n'); + } + + SubmissionResult::response(output.trim_end()) + } + /// Handle system commands that bypass thread-state checks entirely. pub(super) async fn handle_system_command( &self, @@ -480,6 +568,7 @@ impl Agent { " /version Show version info\n", " /tools List available tools\n", " /debug Toggle debug mode\n", + " /reasoning [N|all] Show agent reasoning for turns\n", " /ping Connectivity check\n", "\n", "Jobs:\n", diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index a195458d..fe208c1b 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -63,7 +63,12 @@ impl Agent { ); let system_prompt = if let Some(ws) = self.workspace() { - match ws + let scoped_workspace = if ws.user_id() == message.user_id { + Arc::clone(ws) + } else { + Arc::new(ws.scoped_to_user(&message.user_id)) + }; + match scoped_workspace .system_prompt_for_context_tz(is_group_chat, user_tz) .await { @@ -420,6 +425,19 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { content: Option, reason_ctx: &mut ReasoningContext, ) -> Result, Error> { + // Extract and sanitize the narrative before consuming `content`. + let narrative = content + .as_deref() + .filter(|c| !c.trim().is_empty()) + .map(|c| { + let sanitized = self + .agent + .safety() + .sanitize_tool_output("agent_narrative", c); + sanitized.content + }) + .filter(|c| !c.trim().is_empty()); + // Add the assistant message with tool_calls to context. // OpenAI protocol requires this before tool-result messages. reason_ctx @@ -440,6 +458,41 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { ) .await; + // Build per-tool decisions for the reasoning update. + // Sanitize each rationale through SafetyLayer (parity with JobDelegate). + let decisions: Vec = tool_calls + .iter() + .filter_map(|tc| { + tc.reasoning.as_ref().map(|r| { + let sanitized = self + .agent + .safety() + .sanitize_tool_output("tool_rationale", r) + .content; + crate::channels::ToolDecision { + tool_name: tc.name.clone(), + rationale: sanitized, + } + }) + }) + .collect(); + + // Emit reasoning update to channels. + if narrative.is_some() || !decisions.is_empty() { + let _ = self + .agent + .channels + .send_status( + &self.message.channel, + StatusUpdate::ReasoningUpdate { + narrative: narrative.clone().unwrap_or_default(), + decisions: decisions.clone(), + }, + &self.message.metadata, + ) + .await; + } + // Record tool calls in the thread with sensitive params redacted. { let mut redacted_args: Vec = Vec::with_capacity(tool_calls.len()); @@ -455,8 +508,23 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { if let Some(thread) = sess.threads.get_mut(&self.thread_id) && let Some(turn) = thread.last_turn_mut() { + // Set turn-level narrative. + if turn.narrative.is_none() { + turn.narrative = narrative; + } for (tc, safe_args) in tool_calls.iter().zip(redacted_args) { - turn.record_tool_call(&tc.name, safe_args); + let sanitized_rationale = tc.reasoning.as_ref().map(|r| { + self.agent + .safety() + .sanitize_tool_output("tool_rationale", r) + .content + }); + turn.record_tool_call_with_reasoning( + &tc.name, + safe_args, + sanitized_rationale, + Some(tc.id.clone()), + ); } } } @@ -726,7 +794,7 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { if let Some(thread) = sess.threads.get_mut(&self.thread_id) && let Some(turn) = thread.last_turn_mut() { - turn.record_tool_error(error_msg.clone()); + turn.record_tool_error_for(&tc.id, error_msg.clone()); } } reason_ctx @@ -852,16 +920,19 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { Err(e) => format!("Tool '{}' failed: {}", tc.name, e), }; - // Record sanitized result in thread + // Record sanitized result in thread (identity-based matching). { let mut sess = self.session.lock().await; if let Some(thread) = sess.threads.get_mut(&self.thread_id) && let Some(turn) = thread.last_turn_mut() { if is_tool_error { - turn.record_tool_error(result_content.clone()); + turn.record_tool_error_for(&tc.id, result_content.clone()); } else { - turn.record_tool_result(serde_json::json!(result_content)); + turn.record_tool_result_for( + &tc.id, + serde_json::json!(result_content), + ); } } } @@ -1462,11 +1533,13 @@ mod tests { id: "call_2".to_string(), name: "http".to_string(), arguments: serde_json::json!({"url": "https://example.com"}), + reasoning: None, }, ToolCall { id: "call_3".to_string(), name: "echo".to_string(), arguments: serde_json::json!({"message": "done"}), + reasoning: None, }, ], user_timezone: None, @@ -1652,6 +1725,7 @@ mod tests { id: "call_1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({"message": "hi"}), + reasoning: None, }], ), ChatMessage::tool_result("call_1", "echo", "hi"), @@ -1744,11 +1818,13 @@ mod tests { id: "c1".to_string(), name: "http".to_string(), arguments: serde_json::json!({}), + reasoning: None, }, ToolCall { id: "c2".to_string(), name: "echo".to_string(), arguments: serde_json::json!({}), + reasoning: None, }, ], ), @@ -1782,6 +1858,7 @@ mod tests { id: "c1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({}), + reasoning: None, }], ), ChatMessage::tool_result("c1", "echo", "done"), @@ -1912,6 +1989,7 @@ mod tests { id: crate::llm::generate_tool_call_id(0, 0), name: "echo".to_string(), arguments: serde_json::json!({"message": "looping"}), + reasoning: None, }], input_tokens: 0, output_tokens: 5, @@ -2065,6 +2143,7 @@ mod tests { id: crate::llm::generate_tool_call_id(0, 0), name: "nonexistent_tool".to_string(), arguments: serde_json::json!({}), + reasoning: None, }], input_tokens: 0, output_tokens: 5, diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 7c7ef5f3..a3cdb6cd 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -18,13 +18,14 @@ use std::time::Duration; use chrono::Utc; use regex::Regex; use tokio::sync::{RwLock, mpsc}; +use tokio::task::JoinHandle; use uuid::Uuid; use crate::agent::Scheduler; use crate::agent::routine::{ NotifyConfig, Routine, RoutineAction, RoutineRun, RunStatus, Trigger, next_cron_fire, }; -use crate::channels::OutgoingResponse; +use crate::channels::{IncomingMessage, OutgoingResponse}; use crate::config::RoutineConfig; use crate::context::{JobContext, JobState}; use crate::db::Database; @@ -45,6 +46,11 @@ enum EventMatcher { System { routine: Routine }, } +struct TriggeredRoutine { + routine: Routine, + detail: String, +} + /// Distinguishes why sandbox is unavailable so error messages are accurate. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SandboxReadiness { @@ -56,6 +62,40 @@ pub enum SandboxReadiness { DockerUnavailable, } +/// Check whether an event-triggered routine's user/channel filters match an +/// incoming message. +/// +/// Returns `true` if: +/// - The routine has an `Event` trigger (non-Event routines always return `false`) +/// - The routine's `user_id` matches the message's user scope +/// - The routine's channel filter (if any) matches the message channel +/// case-insensitively +/// +/// This is a pure function extracted from `check_event_triggers` so the +/// filter logic can be unit-tested without async infrastructure. +pub(crate) fn routine_matches_message(routine: &Routine, message: &IncomingMessage) -> bool { + // Only Event-triggered routines can match incoming messages. + if !matches!(routine.trigger, Trigger::Event { .. }) { + return false; + } + + // User ownership filter — only fire routines scoped to this user. + if routine.user_id != message.user_id { + return false; + } + + // Channel filter (case-insensitive, matching emit_system_event behavior) + if let Trigger::Event { + channel: Some(ch), .. + } = &routine.trigger + && !ch.eq_ignore_ascii_case(&message.channel) + { + return false; + } + + true +} + /// The routine execution engine. pub struct RoutineEngine { config: RoutineConfig, @@ -167,10 +207,45 @@ impl RoutineEngine { } /// Check incoming message against event triggers. Returns number of routines fired. + pub async fn check_event_triggers(&self, message: &IncomingMessage, content: &str) -> usize { + let triggered = self.matching_event_triggers(message, content).await; + let fired = triggered.len(); + for triggered in triggered { + std::mem::drop(self.spawn_fire(triggered.routine, "event", Some(triggered.detail))); + } + fired + } + + /// Fire matching event-triggered routines and wait for them to complete. /// - /// Accepts only the three fields needed for matching (user scope, channel, - /// message content) so callers never need to clone a full `IncomingMessage`. - pub async fn check_event_triggers(&self, user_id: &str, channel: &str, content: &str) -> usize { + /// Used by single-message REPL mode so the process does not exit before + /// background event-triggered routines finish. + pub async fn check_event_triggers_and_wait( + &self, + message: &IncomingMessage, + content: &str, + ) -> usize { + let triggered = self.matching_event_triggers(message, content).await; + let fired = triggered.len(); + let handles: Vec> = triggered + .into_iter() + .map(|triggered| self.spawn_fire(triggered.routine, "event", Some(triggered.detail))) + .collect(); + + for handle in handles { + if let Err(e) = handle.await { + tracing::warn!(error = %e, "Event-triggered routine task failed"); + } + } + + fired + } + + async fn matching_event_triggers( + &self, + message: &IncomingMessage, + content: &str, + ) -> Vec { let cache = self.event_cache.read().await; // Early return if there are no message matchers at all. @@ -178,10 +253,9 @@ impl RoutineEngine { .iter() .any(|m| matches!(m, EventMatcher::Message { .. })) { - return 0; + return Vec::new(); } - - let mut fired = 0; + let mut triggered = Vec::new(); // Collect routine IDs for batch query let routine_ids: Vec = cache @@ -193,13 +267,13 @@ impl RoutineEngine { .collect(); if routine_ids.is_empty() { - return 0; + return Vec::new(); } // Single batch query instead of N queries let concurrent_counts = match self.batch_concurrent_counts(&routine_ids).await { Some(counts) => counts, - None => return 0, + None => return Vec::new(), }; for matcher in cache.iter() { @@ -208,16 +282,24 @@ impl RoutineEngine { EventMatcher::System { .. } => continue, }; - if routine.user_id != user_id { - continue; - } - - // Channel filter - if let Trigger::Event { - channel: Some(ch), .. - } = &routine.trigger - && ch != channel - { + // User ownership + channel filter (extracted for testability). + if !routine_matches_message(routine, message) { + // User mismatch is expected for multi-user setups — keep at + // trace to avoid one log per routine per inbound message. + if routine.user_id != message.user_id { + tracing::trace!( + routine = %routine.name, + routine_user = %routine.user_id, + message_user = %message.user_id, + "Skipped: user scope mismatch" + ); + } else { + tracing::debug!( + routine = %routine.name, + channel = %message.channel, + "Skipped: channel mismatch" + ); + } continue; } @@ -228,14 +310,14 @@ impl RoutineEngine { // Cooldown check if !self.check_cooldown(routine) { - tracing::trace!(routine = %routine.name, "Skipped: cooldown active"); + tracing::debug!(routine = %routine.name, "Skipped: cooldown active"); continue; } // Concurrent run check (using batch-loaded counts) let running_count = concurrent_counts.get(&routine.id).copied().unwrap_or(0); if running_count >= routine.guardrails.max_concurrent as i64 { - tracing::trace!(routine = %routine.name, "Skipped: max concurrent reached"); + tracing::debug!(routine = %routine.name, "Skipped: max concurrent reached"); continue; } @@ -246,11 +328,13 @@ impl RoutineEngine { } let detail = truncate(content, 200); - self.spawn_fire(routine.clone(), "event", Some(detail)); - fired += 1; + triggered.push(TriggeredRoutine { + routine: routine.clone(), + detail, + }); } - fired + triggered } /// Emit a structured event to system-event routines. @@ -806,7 +890,12 @@ impl RoutineEngine { } /// Spawn a fire in a background task. - fn spawn_fire(&self, routine: Routine, trigger_type: &str, trigger_detail: Option) { + fn spawn_fire( + &self, + routine: Routine, + trigger_type: &str, + trigger_detail: Option, + ) -> JoinHandle<()> { let run = RoutineRun { id: Uuid::new_v4(), routine_id: routine.id, @@ -843,7 +932,7 @@ impl RoutineEngine { return; } execute_routine(engine, routine, run).await; - }); + }) } fn check_cooldown(&self, routine: &Routine) -> bool { @@ -1502,7 +1591,10 @@ async fn execute_lightweight_with_tools( let force_text = iteration >= max_iterations; if force_text { - // Final iteration: no tools, just get text response + // Final iteration: no tools, just get text response. + // Claude 4.6 rejects assistant prefill; NEAR AI rejects any non-user-ending + // conversation. Ensure the last message is user-role. + crate::util::ensure_ends_with_user_message(&mut messages); let request = CompletionRequest::new(messages) .with_max_tokens(effective_max_tokens) .with_temperature(0.3); @@ -1781,6 +1873,13 @@ pub fn spawn_cron_ticker( engine.check_cron_triggers().await; let mut ticker = tokio::time::interval(interval); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + // Periodic event cache refresh so web/CLI mutations are picked up + // without requiring tool-path code to call refresh_event_cache(). + // Uses wall-clock elapsed time so the refresh cadence is stable + // regardless of the cron tick interval configuration. + let refresh_interval = Duration::from_secs(60); + let mut last_refresh = tokio::time::Instant::now(); loop { ticker.tick().await; @@ -1788,7 +1887,11 @@ pub fn spawn_cron_ticker( // never races with FullJobWatcher instances from this process. engine.sync_dispatched_runs().await; engine.check_cron_triggers().await; - engine.sync_dispatched_runs().await; + + if last_refresh.elapsed() >= refresh_interval { + engine.refresh_event_cache().await; + last_refresh = tokio::time::Instant::now(); + } } }) } @@ -1854,7 +1957,13 @@ fn strip_html_tags(s: &str) -> String { #[cfg(test)] mod tests { - use crate::agent::routine::{NotifyConfig, RunStatus}; + use chrono::Utc; + use uuid::Uuid; + + use crate::agent::routine::{ + NotifyConfig, Routine, RoutineAction, RoutineGuardrails, RunStatus, Trigger, + }; + use crate::channels::IncomingMessage; use crate::config::RoutineConfig; #[test] @@ -2052,6 +2161,117 @@ mod tests { } } + /// Helper to build a test routine with the given user_id and trigger. + fn make_routine(user_id: &str, trigger: Trigger) -> Routine { + Routine { + id: Uuid::new_v4(), + name: "test".to_string(), + description: String::new(), + user_id: user_id.to_string(), + enabled: true, + trigger, + action: RoutineAction::Lightweight { + prompt: String::new(), + context_paths: vec![], + max_tokens: 1000, + use_tools: false, + max_tool_rounds: 0, + }, + guardrails: RoutineGuardrails::default(), + notify: Default::default(), + last_run_at: None, + next_fire_at: None, + run_count: 0, + consecutive_failures: 0, + state: serde_json::Value::Null, + created_at: Utc::now(), + updated_at: Utc::now(), + } + } + + /// Helper to build a test IncomingMessage. + fn make_message(user_id: &str, channel: &str, content: &str) -> IncomingMessage { + IncomingMessage { + id: Uuid::new_v4(), + channel: channel.to_string(), + user_id: user_id.to_string(), + owner_id: user_id.to_string(), + sender_id: user_id.to_string(), + user_name: None, + content: content.to_string(), + thread_id: None, + conversation_scope_id: None, + received_at: Utc::now(), + metadata: serde_json::Value::Null, + timezone: None, + attachments: vec![], + is_internal: false, + } + } + + /// Regression test for issue #1051: event triggers used case-sensitive + /// channel comparison, so "Telegram" != "telegram" caused silent mismatch. + /// Tests the actual `routine_matches_message` function used in `check_event_triggers`. + #[test] + fn test_channel_filter_is_case_insensitive() { + let routine = make_routine( + "user1", + Trigger::Event { + pattern: ".*".to_string(), + channel: Some("Telegram".to_string()), + }, + ); + let msg = make_message("user1", "telegram", "hello"); + + // Case-insensitive channel match must succeed + assert!(super::routine_matches_message(&routine, &msg)); + + // Exact case must also work + let msg_exact = make_message("user1", "Telegram", "hello"); + assert!(super::routine_matches_message(&routine, &msg_exact)); + + // Different channel must not match + let msg_wrong = make_message("user1", "discord", "hello"); + assert!(!super::routine_matches_message(&routine, &msg_wrong)); + } + + /// Regression test for issue #1051: event triggers did not filter by + /// user_id, so routines from user A could fire on messages from user B. + /// Tests the actual `routine_matches_message` function used in `check_event_triggers`. + #[test] + fn test_event_trigger_requires_user_match() { + let routine = make_routine( + "alice", + Trigger::Event { + pattern: ".*".to_string(), + channel: None, + }, + ); + + // Different user must not match + let msg_bob = make_message("bob", "telegram", "hello"); + assert!(!super::routine_matches_message(&routine, &msg_bob)); + + // Same user must match + let msg_alice = make_message("alice", "telegram", "hello"); + assert!(super::routine_matches_message(&routine, &msg_alice)); + } + + /// When no channel filter is set, any channel should match (given user matches). + #[test] + fn test_no_channel_filter_matches_any_channel() { + let routine = make_routine( + "user1", + Trigger::Event { + pattern: ".*".to_string(), + channel: None, + }, + ); + + let msg = make_message("user1", "whatever_channel", "hello"); + assert!(super::routine_matches_message(&routine, &msg)); + } + #[test] fn test_routine_tool_denylist_blocks_self_management_tools() { let denylisted = vec![ diff --git a/src/agent/session.rs b/src/agent/session.rs index 7ec2023f..6c873e46 100644 --- a/src/agent/session.rs +++ b/src/agent/session.rs @@ -449,6 +449,7 @@ impl Thread { id: call_id.clone(), name: tc.name.clone(), arguments: tc.parameters.clone(), + reasoning: None, }) .collect(); @@ -522,7 +523,12 @@ impl Thread { && let Some(ref tcs) = assistant_msg.tool_calls { for tc in tcs { - turn.record_tool_call(&tc.name, tc.arguments.clone()); + turn.record_tool_call_with_reasoning( + &tc.name, + tc.arguments.clone(), + tc.reasoning.clone(), + Some(tc.id.clone()), + ); } } @@ -602,6 +608,10 @@ pub struct Turn { pub completed_at: Option>, /// Error message (if failed). pub error: Option, + /// Agent's reasoning narrative for this turn. + /// Cleaned via `clean_response` and sanitized through `SafetyLayer` before storage. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub narrative: Option, /// Transient image content parts for multimodal LLM input. /// Not serialized — images are only needed for the current LLM call. /// The text description in `user_input` persists for compaction/context. @@ -621,6 +631,7 @@ impl Turn { started_at: Utc::now(), completed_at: None, error: None, + narrative: None, image_content_parts: Vec::new(), } } @@ -656,6 +667,26 @@ impl Turn { parameters: params, result: None, error: None, + rationale: None, + tool_call_id: None, + }); + } + + /// Record a tool call with reasoning context. + pub fn record_tool_call_with_reasoning( + &mut self, + name: impl Into, + params: serde_json::Value, + rationale: Option, + tool_call_id: Option, + ) { + self.tool_calls.push(TurnToolCall { + name: name.into(), + parameters: params, + result: None, + error: None, + rationale, + tool_call_id, }); } @@ -672,6 +703,60 @@ impl Turn { call.error = Some(error.into()); } } + + /// Record a tool result by tool_call_id, with fallback to first pending call. + pub fn record_tool_result_for(&mut self, tool_call_id: &str, result: serde_json::Value) { + if let Some(call) = self + .tool_calls + .iter_mut() + .find(|c| c.tool_call_id.as_deref() == Some(tool_call_id)) + { + call.result = Some(result); + } else if let Some(call) = self + .tool_calls + .iter_mut() + .find(|c| c.result.is_none() && c.error.is_none()) + { + tracing::debug!( + tool_call_id = %tool_call_id, + fallback_tool = %call.name, + "tool_call_id not found, falling back to first pending call" + ); + call.result = Some(result); + } else { + tracing::warn!( + tool_call_id = %tool_call_id, + "Tool result dropped: no matching or pending tool call" + ); + } + } + + /// Record a tool error by tool_call_id, with fallback to first pending call. + pub fn record_tool_error_for(&mut self, tool_call_id: &str, error: impl Into) { + if let Some(call) = self + .tool_calls + .iter_mut() + .find(|c| c.tool_call_id.as_deref() == Some(tool_call_id)) + { + call.error = Some(error.into()); + } else if let Some(call) = self + .tool_calls + .iter_mut() + .find(|c| c.result.is_none() && c.error.is_none()) + { + tracing::debug!( + tool_call_id = %tool_call_id, + fallback_tool = %call.name, + "tool_call_id not found, falling back to first pending call" + ); + call.error = Some(error.into()); + } else { + tracing::warn!( + tool_call_id = %tool_call_id, + "Tool error dropped: no matching or pending tool call" + ); + } + } } /// Record of a tool call made during a turn. @@ -685,6 +770,12 @@ pub struct TurnToolCall { pub result: Option, /// Error from the tool (if failed). pub error: Option, + /// Agent's reasoning for choosing this tool. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub rationale: Option, + /// The tool_call_id from the LLM, for identity-based result matching. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub tool_call_id: Option, } #[cfg(test)] @@ -1309,6 +1400,7 @@ mod tests { id: "call_0".to_string(), name: "search".to_string(), arguments: serde_json::json!({"q": "test"}), + reasoning: None, }; let messages = vec![ ChatMessage::user("Find test"), @@ -1339,6 +1431,7 @@ mod tests { id: "call_0".to_string(), name: "http".to_string(), arguments: serde_json::json!({}), + reasoning: None, }; let messages = vec![ ChatMessage::user("Fetch URL"), @@ -1404,11 +1497,13 @@ mod tests { id: "call_a".to_string(), name: "search".to_string(), arguments: serde_json::json!({"q": "data"}), + reasoning: None, }; let tc2 = ToolCall { id: "call_b".to_string(), name: "write".to_string(), arguments: serde_json::json!({"path": "out.txt"}), + reasoning: None, }; let messages = vec![ ChatMessage::user("Find and save"), @@ -1620,4 +1715,100 @@ mod tests { let merged = thread.drain_pending_messages().unwrap(); assert_eq!(merged, "failed batch\nnew msg"); } + + #[test] + fn test_record_tool_result_for_by_id() { + let mut turn = Turn::new(0, "test"); + turn.record_tool_call_with_reasoning( + "tool_a", + serde_json::json!({}), + None, + Some("id_a".into()), + ); + turn.record_tool_call_with_reasoning( + "tool_b", + serde_json::json!({}), + None, + Some("id_b".into()), + ); + + // Record result for second tool by ID + turn.record_tool_result_for("id_b", serde_json::json!("result_b")); + assert!(turn.tool_calls[0].result.is_none()); + assert_eq!( + turn.tool_calls[1].result.as_ref().unwrap(), + &serde_json::json!("result_b") + ); + } + + #[test] + fn test_record_tool_error_for_by_id() { + let mut turn = Turn::new(0, "test"); + turn.record_tool_call_with_reasoning( + "tool_a", + serde_json::json!({}), + None, + Some("id_a".into()), + ); + turn.record_tool_call_with_reasoning( + "tool_b", + serde_json::json!({}), + None, + Some("id_b".into()), + ); + + turn.record_tool_error_for("id_a", "failed"); + assert_eq!(turn.tool_calls[0].error.as_deref(), Some("failed")); + assert!(turn.tool_calls[1].error.is_none()); + } + + #[test] + fn test_record_tool_result_for_fallback_to_pending() { + let mut turn = Turn::new(0, "test"); + turn.record_tool_call_with_reasoning( + "tool_a", + serde_json::json!({}), + None, + Some("id_a".into()), + ); + turn.record_tool_call_with_reasoning( + "tool_b", + serde_json::json!({}), + None, + Some("id_b".into()), + ); + + // First tool already has a result + turn.tool_calls[0].result = Some(serde_json::json!("done")); + + // Unknown ID should fall back to first pending (tool_b) + turn.record_tool_result_for("unknown_id", serde_json::json!("fallback")); + assert_eq!( + turn.tool_calls[0].result.as_ref().unwrap(), + &serde_json::json!("done") + ); + assert_eq!( + turn.tool_calls[1].result.as_ref().unwrap(), + &serde_json::json!("fallback") + ); + } + + #[test] + fn test_record_tool_result_for_no_pending_is_noop() { + let mut turn = Turn::new(0, "test"); + turn.record_tool_call_with_reasoning( + "tool_a", + serde_json::json!({}), + None, + Some("id_a".into()), + ); + turn.tool_calls[0].result = Some(serde_json::json!("done")); + + // No pending calls, unknown ID — should be a no-op + turn.record_tool_result_for("unknown_id", serde_json::json!("lost")); + assert_eq!( + turn.tool_calls[0].result.as_ref().unwrap(), + &serde_json::json!("done") + ); + } } diff --git a/src/agent/session_manager.rs b/src/agent/session_manager.rs index 3bf20697..ae98b0b0 100644 --- a/src/agent/session_manager.rs +++ b/src/agent/session_manager.rs @@ -102,11 +102,30 @@ impl SessionManager { /// Resolve an external thread ID to an internal thread. /// /// Returns the session and thread ID. Creates both if they don't exist. + /// Delegates to [`resolve_thread_with_parsed_uuid`](Self::resolve_thread_with_parsed_uuid) + /// with `parsed_uuid: None`. pub async fn resolve_thread( &self, user_id: &str, channel: &str, external_thread_id: Option<&str>, + ) -> (Arc>, Uuid) { + self.resolve_thread_with_parsed_uuid(user_id, channel, external_thread_id, None) + .await + } + + /// Like [`resolve_thread`](Self::resolve_thread), but accepts a pre-parsed + /// UUID to skip redundant parsing when the caller has already validated + /// the external thread ID as a UUID (e.g. the approval routing path). + /// + /// Uses a single read-lock acquisition for both the key lookup and the UUID + /// adoption check to reduce contention under concurrent approval load. + pub async fn resolve_thread_with_parsed_uuid( + &self, + user_id: &str, + channel: &str, + external_thread_id: Option<&str>, + parsed_uuid: Option, ) -> (Arc>, Uuid) { let session = self.get_or_create_session(user_id).await; @@ -116,51 +135,65 @@ impl SessionManager { external_thread_id: external_thread_id.map(String::from), }; - // Check if we have a mapping - { + // Use pre-parsed UUID if available, otherwise parse from string. + let ext_uuid = parsed_uuid + .or_else(|| external_thread_id.and_then(|ext_tid| Uuid::parse_str(ext_tid).ok())); + + // Validate that parsed_uuid (if provided) is consistent with external_thread_id. + #[cfg(debug_assertions)] + if let (Some(parsed), Some(ext_tid)) = (&parsed_uuid, external_thread_id) { + debug_assert_eq!( + Uuid::parse_str(ext_tid).ok().as_ref(), + Some(parsed), + "parsed_uuid must be the parsed form of external_thread_id" + ); + } + + // Single read lock for both the key lookup and UUID adoption check + let adoptable_uuid = { let thread_map = self.thread_map.read().await; + + // Fast path: exact key match if let Some(&thread_id) = thread_map.get(&key) { - // Verify thread still exists in session let sess = session.lock().await; if sess.threads.contains_key(&thread_id) { return (Arc::clone(&session), thread_id); } } - } - // Check if external_thread_id is itself a known thread UUID that - // exists in the session but was never registered in the thread_map - // (e.g. created by chat_new_thread_handler or hydrated from DB). - // We only adopt it if no thread_map entry maps to this UUID — - // otherwise it belongs to a different channel scope. - if let Some(ext_tid) = external_thread_id - && let Ok(ext_uuid) = Uuid::parse_str(ext_tid) - { - let thread_map = self.thread_map.read().await; - let mapped_elsewhere = thread_map.values().any(|&v| v == ext_uuid); - drop(thread_map); + // UUID adoption check (still under the same read lock). + // If external_thread_id is a valid UUID not mapped elsewhere, + // it may be a thread created by chat_new_thread_handler or + // hydrated from DB that we can adopt. + // Only attempt adoption when external_thread_id is Some, preserving + // the invariant that None external_thread_id never triggers adoption. + if external_thread_id.is_some() { + ext_uuid.filter(|&uuid| !thread_map.values().any(|&v| v == uuid)) + } else { + None + } + }; // Single read lock dropped here - if !mapped_elsewhere { - let sess = session.lock().await; - if sess.threads.contains_key(&ext_uuid) { - drop(sess); + // If we found an adoptable UUID, verify it exists in session and acquire write lock + if let Some(ext_uuid) = adoptable_uuid { + let sess = session.lock().await; + if sess.threads.contains_key(&ext_uuid) { + drop(sess); - let mut thread_map = self.thread_map.write().await; - // Re-check after acquiring write lock to prevent race condition - // where another task mapped this UUID between our read and write. - if !thread_map.values().any(|&v| v == ext_uuid) { - thread_map.insert(key, ext_uuid); - drop(thread_map); - // Ensure undo manager exists - let mut undo_managers = self.undo_managers.write().await; - undo_managers - .entry(ext_uuid) - .or_insert_with(|| Arc::new(Mutex::new(UndoManager::new()))); - return (session, ext_uuid); - } - // If it was mapped elsewhere while we were unlocked, fall through - // to create a new thread, preserving channel isolation. + let mut thread_map = self.thread_map.write().await; + // Re-check after acquiring write lock to prevent race condition + // where another task mapped this UUID between our read and write. + if !thread_map.values().any(|&v| v == ext_uuid) { + thread_map.insert(key, ext_uuid); + drop(thread_map); + // Ensure undo manager exists + let mut undo_managers = self.undo_managers.write().await; + undo_managers + .entry(ext_uuid) + .or_insert_with(|| Arc::new(Mutex::new(UndoManager::new()))); + return (session, ext_uuid); } + // If mapped elsewhere while unlocked, fall through to create new thread } } @@ -909,6 +942,44 @@ mod tests { } } + #[tokio::test] + async fn test_resolve_thread_consolidates_read_path() { + // Verify that resolve_thread still correctly handles: + // 1. Fast path: key exists in thread_map + // 2. UUID adoption: external_thread_id is a UUID in session but not in map + // 3. New thread: neither path matches + use crate::agent::session::Thread; + + let manager = SessionManager::new(); + + // Case 1: Normal resolution creates thread and maps it + let (session1, tid1) = manager + .resolve_thread("user1", "chan1", Some("ext-1")) + .await; + // Resolving again with same key should return same thread (fast path) + let (_, tid1_again) = manager + .resolve_thread("user1", "chan1", Some("ext-1")) + .await; + assert_eq!(tid1, tid1_again); + + // Case 2: UUID adoption - insert a thread directly into session + let adopted_id = Uuid::new_v4(); + { + let mut sess = session1.lock().await; + let thread = Thread::with_id(adopted_id, sess.id); + sess.threads.insert(adopted_id, thread); + } + // Resolve with the UUID as external_thread_id -- should adopt it + let (_, resolved) = manager + .resolve_thread("user1", "chan1", Some(&adopted_id.to_string())) + .await; + assert_eq!(resolved, adopted_id); + + // Case 3: Different channel gets different thread + let (_, tid2) = manager.resolve_thread("user1", "chan2", None).await; + assert_ne!(tid1, tid2); + } + #[tokio::test] async fn test_resolve_thread_finds_existing_session_thread_by_uuid() { use crate::agent::session::{Session, Thread}; @@ -947,4 +1018,88 @@ mod tests { "should have exactly 1 thread, not a duplicate" ); } + + #[tokio::test] + async fn test_resolve_thread_with_pre_parsed_uuid_adopts_thread() { + use crate::agent::session::Thread; + + let manager = SessionManager::new(); + let (session, _) = manager.resolve_thread("user1", "chan1", None).await; + + // Manually insert a thread with a known UUID + let known_id = Uuid::new_v4(); + { + let mut sess = session.lock().await; + let thread = Thread::with_id(known_id, sess.id); + sess.threads.insert(known_id, thread); + } + + // Resolve with pre-parsed UUID -- should adopt it without re-parsing + let (_, resolved) = manager + .resolve_thread_with_parsed_uuid( + "user1", + "chan1", + Some(&known_id.to_string()), + Some(known_id), + ) + .await; + assert_eq!(resolved, known_id); + } + + #[tokio::test] + async fn test_resolve_thread_with_parsed_uuid_none_delegates_to_parse() { + use crate::agent::session::Thread; + + let manager = SessionManager::new(); + let (session, _) = manager.resolve_thread("user2", "chan2", None).await; + + // Insert a thread with a known UUID + let known_id = Uuid::new_v4(); + { + let mut sess = session.lock().await; + let thread = Thread::with_id(known_id, sess.id); + sess.threads.insert(known_id, thread); + } + + // Resolve with parsed_uuid=None but a valid UUID string -- should + // fall back to parsing the string and still adopt the thread + let (_, resolved) = manager + .resolve_thread_with_parsed_uuid("user2", "chan2", Some(&known_id.to_string()), None) + .await; + assert_eq!(resolved, known_id); + } + + #[tokio::test] + async fn test_resolve_thread_with_none_external_thread_id_does_not_adopt() { + use crate::agent::session::Thread; + + let manager = SessionManager::new(); + let (session, default_tid) = manager.resolve_thread("user3", "chan3", None).await; + + // Manually insert a thread with a known UUID (simulating a thread + // created by chat_new_thread_handler) + let known_id = Uuid::new_v4(); + { + let mut sess = session.lock().await; + let thread = Thread::with_id(known_id, sess.id); + sess.threads.insert(known_id, thread); + } + + // Resolve with external_thread_id=None but parsed_uuid=Some. + // This should NOT adopt the UUID — the old code prevented adoption + // when external_thread_id was None, and we preserve that invariant. + let (_, resolved) = manager + .resolve_thread_with_parsed_uuid("user3", "chan3", None, Some(known_id)) + .await; + + // Should return the existing default thread, not the injected UUID + assert_eq!( + resolved, default_tid, + "should return existing default thread when external_thread_id is None" + ); + assert_ne!( + resolved, known_id, + "should NOT adopt UUID when external_thread_id is None" + ); + } } diff --git a/src/agent/submission.rs b/src/agent/submission.rs index 8594c969..5a81e0bf 100644 --- a/src/agent/submission.rs +++ b/src/agent/submission.rs @@ -92,6 +92,17 @@ impl SubmissionParser { args: vec![], }; } + if lower == "/reasoning" || lower.starts_with("/reasoning ") { + let args: Vec = trimmed + .split_whitespace() + .skip(1) + .map(|s| s.to_string()) + .collect(); + return Submission::SystemCommand { + command: "reasoning".to_string(), + args, + }; + } if lower == "/restart" { tracing::debug!("[SubmissionParser::parse] Recognized /restart command"); return Submission::SystemCommand { diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index b2820e7e..11f211f9 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -513,10 +513,10 @@ impl Agent { }; thread.complete_turn(&response); - let (turn_number, tool_calls) = thread + let (turn_number, tool_calls, narrative) = thread .turns .last() - .map(|t| (t.turn_number, t.tool_calls.clone())) + .map(|t| (t.turn_number, t.tool_calls.clone(), t.narrative.clone())) .unwrap_or_default(); let _ = self .channels @@ -534,6 +534,7 @@ impl Agent { &message.user_id, turn_number, &tool_calls, + narrative.as_deref(), ) .await; self.persist_assistant_response( @@ -725,7 +726,9 @@ impl Agent { /// /// Stored between the user and assistant messages so that /// `build_turns_from_db_messages` can reconstruct the tool call history. - /// Content is a JSON array of tool call summaries. + /// Content is a JSON object: `{ "calls": [...], "narrative": "..." }`. + /// The `calls` array contains tool call summaries with optional `rationale` + /// and `tool_call_id` fields. Legacy rows may be plain JSON arrays. pub(super) async fn persist_tool_calls( &self, thread_id: Uuid, @@ -733,6 +736,7 @@ impl Agent { user_id: &str, turn_number: usize, tool_calls: &[crate::agent::session::TurnToolCall], + narrative: Option<&str>, ) { if tool_calls.is_empty() { return; @@ -767,11 +771,30 @@ impl Agent { if let Some(ref error) = tc.error { obj["error"] = serde_json::Value::String(truncate_preview(error, 200)); } + if let Some(ref rationale) = tc.rationale { + obj["rationale"] = serde_json::Value::String(truncate_preview(rationale, 500)); + } + if let Some(ref tool_call_id) = tc.tool_call_id { + obj["tool_call_id"] = + serde_json::Value::String(truncate_preview(tool_call_id, 128)); + } obj }) .collect(); - let content = match serde_json::to_string(&summaries) { + // Wrap in an object with optional narrative so it can be reconstructed. + // safety: no byte-index slicing here; comment describes JSON shape + let wrapper = if let Some(n) = narrative { + serde_json::json!({ + "narrative": truncate_preview(n, 1000), + "calls": summaries, + }) + } else { + serde_json::json!({ + "calls": summaries, + }) + }; + let content = match serde_json::to_string(&wrapper) { Ok(c) => c, Err(e) => { tracing::warn!("Failed to serialize tool calls: {}", e); @@ -1104,9 +1127,12 @@ impl Agent { && let Some(turn) = thread.last_turn_mut() { if is_tool_error { - turn.record_tool_error(result_content.clone()); + turn.record_tool_error_for(&pending.tool_call_id, result_content.clone()); } else { - turn.record_tool_result(serde_json::json!(result_content)); + turn.record_tool_result_for( + &pending.tool_call_id, + serde_json::json!(result_content), + ); } } } @@ -1358,9 +1384,12 @@ impl Agent { && let Some(turn) = thread.last_turn_mut() { if is_deferred_error { - turn.record_tool_error(deferred_content.clone()); + turn.record_tool_error_for(&tc.id, deferred_content.clone()); } else { - turn.record_tool_result(serde_json::json!(deferred_content)); + turn.record_tool_result_for( + &tc.id, + serde_json::json!(deferred_content), + ); } } } @@ -1459,10 +1488,10 @@ impl Agent { let (response, suggestions) = crate::agent::dispatcher::extract_suggestions(&response); thread.complete_turn(&response); - let (turn_number, tool_calls) = thread + let (turn_number, tool_calls, narrative) = thread .turns .last() - .map(|t| (t.turn_number, t.tool_calls.clone())) + .map(|t| (t.turn_number, t.tool_calls.clone(), t.narrative.clone())) .unwrap_or_default(); // User message already persisted at turn start; save tool calls then assistant response self.persist_tool_calls( @@ -1471,6 +1500,7 @@ impl Agent { &message.user_id, turn_number, &tool_calls, + narrative.as_deref(), ) .await; self.persist_assistant_response( @@ -1816,7 +1846,20 @@ fn rebuild_chat_messages_from_db( "assistant" => result.push(ChatMessage::assistant(&msg.content)), "tool_calls" => { // Try to parse the enriched JSON and rebuild tool messages. - if let Ok(calls) = serde_json::from_str::>(&msg.content) { + // Supports two formats: + // - Old: plain JSON array of tool call summaries + // - New: wrapped object { "calls": [...], "narrative": "..." } + let calls: Vec = + match serde_json::from_str::(&msg.content) { + Ok(serde_json::Value::Array(arr)) => arr, + Ok(serde_json::Value::Object(obj)) => obj + .get("calls") + .and_then(|v| v.as_array()) + .cloned() + .unwrap_or_default(), + _ => Vec::new(), + }; + { if calls.is_empty() { continue; } @@ -1839,6 +1882,10 @@ fn rebuild_chat_messages_from_db( .get("parameters") .cloned() .unwrap_or(serde_json::json!({})), + reasoning: c + .get("rationale") + .and_then(|v| v.as_str()) + .map(String::from), }) .collect(); diff --git a/src/app.rs b/src/app.rs index edd547d3..074e9479 100644 --- a/src/app.rs +++ b/src/app.rs @@ -312,13 +312,7 @@ impl AppBuilder { .create_provider(&self.config.llm.nearai.base_url, self.session.clone()); // Register memory tools if database is available - let workspace_user_id = self - .config - .channels - .gateway - .as_ref() - .map(|gw| gw.user_id.as_str()) - .unwrap_or("default"); + let workspace_user_id = self.config.owner_id.as_str(); let workspace = if let Some(ref db) = self.db { let emb_cache_config = EmbeddingCacheConfig { max_entries: self.config.embeddings.cache_size, diff --git a/src/bridge/llm_adapter.rs b/src/bridge/llm_adapter.rs index 91f6d81a..5f484723 100644 --- a/src/bridge/llm_adapter.rs +++ b/src/bridge/llm_adapter.rs @@ -184,6 +184,7 @@ fn thread_msg_to_chat(msg: &ThreadMessage) -> ChatMessage { id: c.id.clone(), name: c.action_name.clone(), arguments: c.parameters.clone(), + reasoning: None, }) .collect(), ); diff --git a/src/channels/channel.rs b/src/channels/channel.rs index 9bcee12e..784b6bcf 100644 --- a/src/channels/channel.rs +++ b/src/channels/channel.rs @@ -265,6 +265,15 @@ impl OutgoingResponse { } } +/// A single tool decision within a reasoning update. +#[derive(Debug, Clone)] +pub struct ToolDecision { + /// Tool name. + pub tool_name: String, + /// Agent's reasoning for choosing this tool. + pub rationale: String, +} + /// Status update types for showing agent activity. #[derive(Debug, Clone)] pub enum StatusUpdate { @@ -333,6 +342,13 @@ pub enum StatusUpdate { }, /// Suggested follow-up messages for the user. Suggestions { suggestions: Vec }, + /// Agent reasoning update (why it chose specific tools). + ReasoningUpdate { + /// Human-readable summary of the agent's decision. + narrative: String, + /// Per-tool decisions. + decisions: Vec, + }, /// Per-turn token usage and cost summary (shown as subtle metadata). TurnCost { input_tokens: u64, diff --git a/src/channels/mod.rs b/src/channels/mod.rs index c0230692..46e25514 100644 --- a/src/channels/mod.rs +++ b/src/channels/mod.rs @@ -39,7 +39,7 @@ mod webhook_server; pub use channel::{ AttachmentKind, Channel, ChannelSecretUpdater, IncomingAttachment, IncomingMessage, - MessageStream, OutgoingResponse, StatusUpdate, routing_target_from_metadata, + MessageStream, OutgoingResponse, StatusUpdate, ToolDecision, routing_target_from_metadata, }; pub use http::{HttpChannel, HttpChannelState}; pub use manager::ChannelManager; diff --git a/src/channels/repl.rs b/src/channels/repl.rs index 055dc3ad..41d73a8c 100644 --- a/src/channels/repl.rs +++ b/src/channels/repl.rs @@ -75,6 +75,7 @@ const SLASH_COMMANDS: &[&str] = &[ "/suggest", "/thread", "/resume", + "/reasoning", ]; /// Rustyline helper for slash-command tab completion. @@ -430,6 +431,18 @@ impl ReplChannel { let _ = execute!(stderr, terminal::Clear(terminal::ClearType::FromCursorDown)); } } + + async fn finish_single_message_turn(&self) { + if self.single_message.is_none() { + return; + } + + let tx = self.msg_tx.lock().ok().and_then(|mut guard| guard.take()); + if let Some(tx) = tx { + let msg = IncomingMessage::new("repl", &self.user_id, "/quit"); + let _ = tx.send(msg).await; + } + } } impl Default for ReplChannel { @@ -479,7 +492,9 @@ impl Channel for ReplChannel { async fn start(&self) -> Result { let (tx, rx) = mpsc::channel(32); - // Store tx so send_status can inject approval responses directly + // Approval prompts inject responses back through this sender. + // In single-message mode we keep it until the turn finishes, then + // drop it after enqueuing /quit so the receiver stream can close. if let Ok(mut guard) = self.msg_tx.lock() { *guard = Some(tx.clone()); } @@ -495,11 +510,10 @@ impl Channel for ReplChannel { // Single message mode: send it and return if let Some(msg) = single_message { - let incoming = IncomingMessage::new("repl", &user_id, &msg).with_timezone(&sys_tz); + let incoming = IncomingMessage::new("repl", &user_id, &msg) + .with_metadata(serde_json::json!({ "single_message_mode": true })) + .with_timezone(&sys_tz); let _ = tx.blocking_send(incoming); - // Ensure the agent exits after handling exactly one turn in -m mode, - // even when other channels (gateway/http) are enabled. - let _ = tx.blocking_send(IncomingMessage::new("repl", &user_id, "/quit")); return; } @@ -662,6 +676,7 @@ impl Channel for ReplChannel { println!(); println!(); self.stdin_locked.store(false, Ordering::Relaxed); + self.finish_single_message_turn().await; return Ok(()); } @@ -680,6 +695,7 @@ impl Channel for ReplChannel { println!(); // Unlock stdin so readline can resume self.stdin_locked.store(false, Ordering::Relaxed); + self.finish_single_message_turn().await; Ok(()) } @@ -779,6 +795,7 @@ impl Channel for ReplChannel { let msg_tx = Arc::clone(&self.msg_tx); let user_id = self.user_id.clone(); let lock_flag = Arc::clone(&self.stdin_locked); + let single_message_mode = self.single_message.is_some(); tokio::task::spawn_blocking(move || { let action = run_approval_selector(allow_always).unwrap_or("n"); // Unlock stdin so readline can resume after approval @@ -787,7 +804,12 @@ impl Channel for ReplChannel { return; }; if let Some(tx) = guard.as_ref() { - let msg = IncomingMessage::new("repl", &user_id, action); + let msg = if single_message_mode { + IncomingMessage::new("repl", &user_id, action) + .with_metadata(serde_json::json!({ "single_message_mode": true })) + } else { + IncomingMessage::new("repl", &user_id, action) + }; let _ = tx.blocking_send(msg); } }); @@ -841,6 +863,19 @@ impl Channel for ReplChannel { StatusUpdate::Suggestions { .. } => { // Suggestions are only rendered by the web gateway } + StatusUpdate::ReasoningUpdate { + narrative, + decisions, + } => { + if !narrative.is_empty() { + let display = truncate_for_preview(&narrative, CLI_STATUS_MAX); + eprintln!(" \x1b[94m\u{25B6} {display}\x1b[0m"); + } + for d in &decisions { + let display = truncate_for_preview(&d.rationale, CLI_STATUS_MAX); + eprintln!(" \x1b[90m\u{2192} {}: {display}\x1b[0m", d.tool_name); + } + } StatusUpdate::TurnCost { .. } => { // Cost display is handled by the TUI channel } @@ -875,6 +910,7 @@ impl Channel for ReplChannel { #[cfg(test)] mod tests { use futures::StreamExt; + use tokio::time::{Duration, timeout}; use super::*; @@ -883,16 +919,36 @@ mod tests { let repl = ReplChannel::with_message("hi".to_string()); let mut stream = repl.start().await.expect("repl start should succeed"); - let first = stream.next().await.expect("first message missing"); + let first = timeout(Duration::from_secs(1), stream.next()) + .await + .expect("timed out waiting for first message") + .expect("first message missing"); assert_eq!(first.channel, "repl"); assert_eq!(first.content, "hi"); - let second = stream.next().await.expect("quit message missing"); + assert!( + timeout(Duration::from_millis(100), stream.next()) + .await + .is_err(), + "single-message mode should wait for the turn to finish before quitting" + ); + + repl.respond(&first, OutgoingResponse::text("done")) + .await + .expect("respond should succeed"); + + let second = timeout(Duration::from_secs(1), stream.next()) + .await + .expect("timed out waiting for quit message") + .expect("quit message missing"); assert_eq!(second.channel, "repl"); assert_eq!(second.content, "/quit"); assert!( - stream.next().await.is_none(), + timeout(Duration::from_secs(1), stream.next()) + .await + .expect("timed out waiting for stream to close") + .is_none(), "stream should end after /quit" ); } diff --git a/src/channels/wasm/wrapper.rs b/src/channels/wasm/wrapper.rs index 65e4de88..a0f9689f 100644 --- a/src/channels/wasm/wrapper.rs +++ b/src/channels/wasm/wrapper.rs @@ -3061,6 +3061,20 @@ fn status_to_wit( }, // Suggestions and turn cost are web-gateway-only; skip for WASM channels StatusUpdate::Suggestions { .. } | StatusUpdate::TurnCost { .. } => return None, + StatusUpdate::ReasoningUpdate { + narrative, + decisions, + } => { + let mut msg = narrative.clone(); + for d in decisions { + msg.push_str(&format!("\n → {}: {}", d.tool_name, d.rationale)); + } + wit_channel::StatusUpdate { + status: wit_channel::StatusType::Status, + message: msg, + metadata_json, + } + } }) } diff --git a/src/channels/web/handlers/chat.rs b/src/channels/web/handlers/chat.rs index e11ca38d..66e39a6e 100644 --- a/src/channels/web/handlers/chat.rs +++ b/src/channels/web/handlers/chat.rs @@ -416,8 +416,10 @@ pub async fn chat_history_handler( truncate_preview(&s, 500) }), error: tc.error.clone(), + rationale: tc.rationale.clone(), }) .collect(), + narrative: t.narrative.clone(), }) .collect(); diff --git a/src/channels/web/handlers/routines.rs b/src/channels/web/handlers/routines.rs index d27adca2..fc56b187 100644 --- a/src/channels/web/handlers/routines.rs +++ b/src/channels/web/handlers/routines.rs @@ -114,7 +114,7 @@ pub async fn routines_detail_handler( trigger_type: run.trigger_type.clone(), started_at: run.started_at.to_rfc3339(), completed_at: run.completed_at.map(|dt| dt.to_rfc3339()), - status: format!("{:?}", run.status), + status: run.status.to_string(), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, job_id: run.job_id, @@ -324,7 +324,7 @@ pub async fn routines_runs_handler( trigger_type: run.trigger_type.clone(), started_at: run.started_at.to_rfc3339(), completed_at: run.completed_at.map(|dt| dt.to_rfc3339()), - status: format!("{:?}", run.status), + status: run.status.to_string(), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, job_id: run.job_id, diff --git a/src/channels/web/mod.rs b/src/channels/web/mod.rs index a44d9979..63aedaa0 100644 --- a/src/channels/web/mod.rs +++ b/src/channels/web/mod.rs @@ -98,7 +98,8 @@ impl GatewayChannel { job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: config.user_id.clone(), + owner_id: config.user_id.clone(), + default_sender_id: config.user_id.clone(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(ws::WsConnectionTracker::new())), llm_provider: None, @@ -121,6 +122,22 @@ impl GatewayChannel { } } + /// Rebind the single-user auth identity to the durable owner scope while + /// preserving the configured gateway sender/routing identity. + pub fn with_owner_scope(mut self, owner_id: impl Into) -> Self { + let owner_id = owner_id.into(); + let single_user_token = if self.config.user_tokens.is_none() { + self.auth.first_token().map(ToOwned::to_owned) + } else { + None + }; + if let Some(token) = single_user_token { + self.auth = MultiAuthState::single(token, owner_id.clone()); + } + self.rebuild_state(|s| s.owner_id = owner_id); + self + } + /// Create a gateway channel with a pre-built multi-user auth state. pub fn new_multi_auth(config: GatewayConfig, auth: MultiAuthState) -> Self { let state = Arc::new(GatewayState { @@ -137,7 +154,8 @@ impl GatewayChannel { job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: config.user_id.clone(), + owner_id: config.user_id.clone(), + default_sender_id: config.user_id.clone(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(ws::WsConnectionTracker::new())), llm_provider: None, @@ -177,7 +195,8 @@ impl GatewayChannel { job_manager: self.state.job_manager.clone(), prompt_queue: self.state.prompt_queue.clone(), scheduler: self.state.scheduler.clone(), - default_user_id: self.state.default_user_id.clone(), + owner_id: self.state.owner_id.clone(), + default_sender_id: self.state.default_sender_id.clone(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: self.state.ws_tracker.clone(), llm_provider: self.state.llm_provider.clone(), @@ -470,6 +489,20 @@ impl Channel for GatewayChannel { }, StatusUpdate::Suggestions { suggestions } => AppEvent::Suggestions { suggestions, + thread_id: thread_id.clone(), + }, + StatusUpdate::ReasoningUpdate { + narrative, + decisions, + } => AppEvent::ReasoningUpdate { + narrative, + decisions: decisions + .into_iter() + .map(|d| crate::channels::web::types::ToolDecisionDto { + tool_name: d.tool_name, + rationale: d.rationale, + }) + .collect(), thread_id, }, StatusUpdate::TurnCost { diff --git a/src/channels/web/openai_compat.rs b/src/channels/web/openai_compat.rs index 55b7c854..0c0f1a9e 100644 --- a/src/channels/web/openai_compat.rs +++ b/src/channels/web/openai_compat.rs @@ -231,6 +231,7 @@ pub fn convert_messages(messages: &[OpenAiMessage]) -> Result, name: tc.function.name.clone(), arguments: serde_json::from_str(&tc.function.arguments) .unwrap_or(serde_json::Value::Object(Default::default())), + reasoning: None, }) .collect(); Ok(ChatMessage::assistant_with_tool_calls( @@ -954,6 +955,7 @@ mod tests { id: "call_abc".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "rust"}), + reasoning: None, }]; let converted = convert_tool_calls_to_openai(&calls); diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index f81e703b..c24ceb16 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -345,8 +345,10 @@ pub struct GatewayState { pub job_manager: Option>, /// Prompt queue for Claude Code follow-up prompts. pub prompt_queue: Option, - /// Default user ID (fallback for non-request contexts like heartbeat/routines). - pub default_user_id: String, + /// Durable owner scope for persistence and unauthenticated callback flows. + pub owner_id: String, + /// Default sender/routing identity for gateway-originated messages. + pub default_sender_id: String, /// Shutdown signal sender. pub shutdown_tx: tokio::sync::RwLock>>, /// WebSocket connection tracker. @@ -775,7 +777,7 @@ async fn oauth_callback_handler( error = %error, "OAuth callback received with malformed state" ); - clear_auth_mode(&state, &state.default_user_id).await; + clear_auth_mode(&state, &state.owner_id).await; return oauth_error_page("IronClaw"); } }; @@ -949,7 +951,7 @@ async fn oauth_callback_handler( message }; - // Broadcast SSE event to notify the web UI + // Broadcast event to notify the web UI if let Some(ref sse) = flow.sse_manager { sse.broadcast_for_user( &flow.user_id, @@ -1136,7 +1138,7 @@ async fn slack_relay_oauth_callback_handler( let state_key = format!("relay:{}:oauth_state", DEFAULT_RELAY_NAME); let stored_state = match ext_mgr .secrets() - .get_decrypted(&state.default_user_id, &state_key) + .get_decrypted(&state.owner_id, &state_key) .await { Ok(secret) => secret.expose().to_string(), @@ -1160,10 +1162,7 @@ async fn slack_relay_oauth_callback_handler( } // Delete the nonce (one-time use) - let _ = ext_mgr - .secrets() - .delete(&state.default_user_id, &state_key) - .await; + let _ = ext_mgr.secrets().delete(&state.owner_id, &state_key).await; let result: Result<(), String> = async { let store = state.store.as_ref().ok_or_else(|| { @@ -1174,16 +1173,12 @@ async fn slack_relay_oauth_callback_handler( // Store team_id in settings let team_id_key = format!("relay:{}:team_id", DEFAULT_RELAY_NAME); let _ = store - .set_setting( - &state.default_user_id, - &team_id_key, - &serde_json::json!(team_id), - ) + .set_setting(&state.owner_id, &team_id_key, &serde_json::json!(team_id)) .await; // Activate the relay channel ext_mgr - .activate_stored_relay(DEFAULT_RELAY_NAME, &state.default_user_id) + .activate_stored_relay(DEFAULT_RELAY_NAME, &state.owner_id) .await .map_err(|e| format!("Failed to activate relay channel: {}", e))?; @@ -1202,7 +1197,7 @@ async fn slack_relay_oauth_callback_handler( } }; - // Broadcast SSE event to notify the web UI + // Broadcast event to notify the web UI state.sse.broadcast(AppEvent::AuthCompleted { extension_name: DEFAULT_RELAY_NAME.to_string(), success, @@ -1303,6 +1298,9 @@ async fn chat_send_handler( } let mut msg = IncomingMessage::new("gateway", &user.user_id, &req.content); + if state.owner_id != state.default_sender_id && user.user_id == state.owner_id { + msg = msg.with_sender_id(&state.default_sender_id); + } // Prefer timezone from JSON body, fall back to X-Timezone header let tz = req .timezone @@ -1404,6 +1402,9 @@ async fn chat_approval_handler( })?; let mut msg = IncomingMessage::new("gateway", &user.user_id, content); + if state.owner_id != state.default_sender_id && user.user_id == state.owner_id { + msg = msg.with_sender_id(&state.default_sender_id); + } if let Some(ref thread_id) = req.thread_id { msg = msg.with_thread(thread_id); @@ -1724,8 +1725,10 @@ async fn chat_history_handler( truncate_preview(&s, 500) }), error: tc.error.clone(), + rationale: tc.rationale.clone(), }) .collect(), + narrative: t.narrative.clone(), }) .collect(); @@ -2572,7 +2575,7 @@ async fn routines_runs_handler( trigger_type: run.trigger_type.clone(), started_at: run.started_at.to_rfc3339(), completed_at: run.completed_at.map(|dt| dt.to_rfc3339()), - status: format!("{:?}", run.status), + status: run.status.to_string(), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, job_id: run.job_id, @@ -2976,7 +2979,8 @@ mod tests { store: None, job_manager: None, prompt_queue: None, - default_user_id: "test".to_string(), + owner_id: "test".to_string(), + default_sender_id: "test".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: None, llm_provider: None, diff --git a/src/channels/web/sse.rs b/src/channels/web/sse.rs index 81d9de9a..e36cceab 100644 --- a/src/channels/web/sse.rs +++ b/src/channels/web/sse.rs @@ -186,30 +186,7 @@ impl SseManager { return None; } }; - let event_type = match &event { - AppEvent::Response { .. } => "response", - AppEvent::Thinking { .. } => "thinking", - AppEvent::ToolStarted { .. } => "tool_started", - AppEvent::ToolCompleted { .. } => "tool_completed", - AppEvent::ToolResult { .. } => "tool_result", - AppEvent::StreamChunk { .. } => "stream_chunk", - AppEvent::Status { .. } => "status", - AppEvent::ApprovalNeeded { .. } => "approval_needed", - AppEvent::AuthRequired { .. } => "auth_required", - AppEvent::AuthCompleted { .. } => "auth_completed", - AppEvent::Error { .. } => "error", - AppEvent::JobStarted { .. } => "job_started", - AppEvent::JobMessage { .. } => "job_message", - AppEvent::JobToolUse { .. } => "job_tool_use", - AppEvent::JobToolResult { .. } => "job_tool_result", - AppEvent::JobStatus { .. } => "job_status", - AppEvent::JobResult { .. } => "job_result", - AppEvent::Heartbeat => "heartbeat", - AppEvent::ImageGenerated { .. } => "image_generated", - AppEvent::Suggestions { .. } => "suggestions", - AppEvent::TurnCost { .. } => "turn_cost", - AppEvent::ExtensionStatus { .. } => "extension_status", - }; + let event_type = event.event_type(); Some(Ok(Event::default().event(event_type).data(data))) }); diff --git a/src/channels/web/static/app.js b/src/channels/web/static/app.js index ddcfc828..6b366482 100644 --- a/src/channels/web/static/app.js +++ b/src/channels/web/static/app.js @@ -4265,9 +4265,9 @@ function renderRoutineDetail(routine) { + 'TriggerStartedCompletedStatusSummaryTokens' + ''; for (const run of routine.recent_runs) { - const runStatusClass = run.status === 'Ok' ? 'completed' - : run.status === 'Failed' ? 'failed' - : run.status === 'Attention' ? 'stuck' + const runStatusClass = run.status === 'ok' ? 'completed' + : run.status === 'failed' ? 'failed' + : run.status === 'attention' ? 'stuck' : 'in_progress'; html += '' + '' + escapeHtml(run.trigger_type) + '' diff --git a/src/channels/web/test_helpers.rs b/src/channels/web/test_helpers.rs index 802512a6..0f7e5d12 100644 --- a/src/channels/web/test_helpers.rs +++ b/src/channels/web/test_helpers.rs @@ -76,7 +76,8 @@ impl TestGatewayBuilder { store: None, job_manager: None, prompt_queue: None, - default_user_id: self.user_id, + owner_id: self.user_id.clone(), + default_sender_id: self.user_id, shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: self.llm_provider, diff --git a/src/channels/web/tests/multi_tenant.rs b/src/channels/web/tests/multi_tenant.rs index 55010831..335f841c 100644 --- a/src/channels/web/tests/multi_tenant.rs +++ b/src/channels/web/tests/multi_tenant.rs @@ -16,6 +16,7 @@ use axum::routing::{delete, get, post}; use tower::ServiceExt; use uuid::Uuid; +use crate::channels::web::GatewayChannel; use crate::channels::web::auth::{ AuthenticatedUser, MultiAuthState, UserIdentity, auth_middleware, }; @@ -23,6 +24,7 @@ use crate::channels::web::server::{ ActiveConfigSnapshot, GatewayState, PerUserRateLimiter, PromptQueue, RateLimiter, WorkspacePool, }; use crate::channels::web::sse::SseManager; +use crate::config::GatewayConfig; // ── Helpers ──────────────────────────────────────────────────────────── @@ -64,7 +66,8 @@ fn build_state( store, job_manager: None, prompt_queue, - default_user_id: "test".to_string(), + owner_id: "test".to_string(), + default_sender_id: "test".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: None, llm_provider: None, @@ -82,6 +85,40 @@ fn build_state( }) } +fn gateway_config() -> GatewayConfig { + GatewayConfig { + host: "127.0.0.1".to_string(), + port: 3000, + auth_token: Some("gateway-auth".to_string()), + user_id: "gateway-sender".to_string(), + workspace_read_scopes: Vec::new(), + memory_layers: Vec::new(), + user_tokens: None, + } +} + +#[test] +fn with_owner_scope_updates_gateway_owner_scope_in_multi_user_mode() { + let mut gateway = GatewayChannel::new(gateway_config()); + gateway.auth = two_user_auth(); + gateway.config.user_tokens = Some(HashMap::new()); + let gateway = gateway.with_owner_scope("owner-scope"); + + assert_eq!(gateway.state.owner_id, "owner-scope"); + assert_eq!(gateway.state.default_sender_id, "gateway-sender"); + + let alice = gateway + .auth + .authenticate("tok-alice") + .expect("alice token should remain valid"); + let bob = gateway + .auth + .authenticate("tok-bob") + .expect("bob token should remain valid"); + assert_eq!(alice.user_id, "alice"); + assert_eq!(bob.user_id, "bob"); +} + /// Create a libSQL-backed test database in a temporary directory. /// /// Returns the database and a `TempDir` guard — the database file is diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index 62b59d98..54288436 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -63,6 +63,9 @@ pub struct TurnInfo { pub started_at: String, pub completed_at: Option, pub tool_calls: Vec, + /// Agent's reasoning narrative for this turn. + #[serde(skip_serializing_if = "Option::is_none")] + pub narrative: Option, } #[derive(Debug, Serialize)] @@ -74,6 +77,9 @@ pub struct ToolCallInfo { pub result_preview: Option, #[serde(skip_serializing_if = "Option::is_none")] pub error: Option, + /// Agent's reasoning for choosing this tool. + #[serde(skip_serializing_if = "Option::is_none")] + pub rationale: Option, } #[derive(Debug, Serialize)] @@ -116,7 +122,7 @@ pub struct ApprovalRequest { // --- App Event (re-exported from ironclaw_common) --- -pub use ironclaw_common::AppEvent; +pub use ironclaw_common::{AppEvent, ToolDecisionDto}; // --- Memory --- @@ -630,30 +636,7 @@ pub enum WsServerMessage { impl WsServerMessage { /// Create a WsServerMessage from an AppEvent. pub fn from_app_event(event: &AppEvent) -> Self { - let event_type = match event { - AppEvent::Response { .. } => "response", - AppEvent::Thinking { .. } => "thinking", - AppEvent::ToolStarted { .. } => "tool_started", - AppEvent::ToolCompleted { .. } => "tool_completed", - AppEvent::ToolResult { .. } => "tool_result", - AppEvent::StreamChunk { .. } => "stream_chunk", - AppEvent::Status { .. } => "status", - AppEvent::JobStarted { .. } => "job_started", - AppEvent::ApprovalNeeded { .. } => "approval_needed", - AppEvent::AuthRequired { .. } => "auth_required", - AppEvent::AuthCompleted { .. } => "auth_completed", - AppEvent::Error { .. } => "error", - AppEvent::Heartbeat => "heartbeat", - AppEvent::JobMessage { .. } => "job_message", - AppEvent::JobToolUse { .. } => "job_tool_use", - AppEvent::JobToolResult { .. } => "job_tool_result", - AppEvent::JobStatus { .. } => "job_status", - AppEvent::JobResult { .. } => "job_result", - AppEvent::ImageGenerated { .. } => "image_generated", - AppEvent::Suggestions { .. } => "suggestions", - AppEvent::TurnCost { .. } => "turn_cost", - AppEvent::ExtensionStatus { .. } => "extension_status", - }; + let event_type = event.event_type(); let data = serde_json::to_value(event).unwrap_or(serde_json::Value::Null); WsServerMessage::Event { event_type: event_type.to_string(), @@ -945,12 +928,12 @@ mod tests { } #[test] - fn test_ws_server_from_sse_response() { - let sse = AppEvent::Response { + fn test_ws_server_from_app_event_response() { + let event = AppEvent::Response { content: "hello".to_string(), thread_id: "t1".to_string(), }; - let ws = WsServerMessage::from_app_event(&sse); + let ws = WsServerMessage::from_app_event(&event); match ws { WsServerMessage::Event { event_type, data } => { assert_eq!(event_type, "response"); @@ -962,12 +945,12 @@ mod tests { } #[test] - fn test_ws_server_from_sse_thinking() { - let sse = AppEvent::Thinking { + fn test_ws_server_from_app_event_thinking() { + let event = AppEvent::Thinking { message: "reasoning...".to_string(), thread_id: None, }; - let ws = WsServerMessage::from_app_event(&sse); + let ws = WsServerMessage::from_app_event(&event); match ws { WsServerMessage::Event { event_type, data } => { assert_eq!(event_type, "thinking"); @@ -978,8 +961,8 @@ mod tests { } #[test] - fn test_ws_server_from_sse_approval_needed() { - let sse = AppEvent::ApprovalNeeded { + fn test_ws_server_from_app_event_approval_needed() { + let event = AppEvent::ApprovalNeeded { request_id: "r1".to_string(), tool_name: "shell".to_string(), description: "Run ls".to_string(), @@ -987,7 +970,7 @@ mod tests { thread_id: Some("t1".to_string()), allow_always: true, }; - let ws = WsServerMessage::from_app_event(&sse); + let ws = WsServerMessage::from_app_event(&event); match ws { WsServerMessage::Event { event_type, data } => { assert_eq!(event_type, "approval_needed"); @@ -999,9 +982,9 @@ mod tests { } #[test] - fn test_ws_server_from_sse_heartbeat() { - let sse = AppEvent::Heartbeat; - let ws = WsServerMessage::from_app_event(&sse); + fn test_ws_server_from_app_event_heartbeat() { + let event = AppEvent::Heartbeat; + let ws = WsServerMessage::from_app_event(&event); match ws { WsServerMessage::Event { event_type, .. } => { assert_eq!(event_type, "heartbeat"); @@ -1041,7 +1024,7 @@ mod tests { } #[test] - fn test_sse_auth_required_serialize() { + fn test_app_event_auth_required_serialize() { let event = AppEvent::AuthRequired { extension_name: "notion".to_string(), instructions: Some("Get your token from...".to_string()), @@ -1058,7 +1041,7 @@ mod tests { } #[test] - fn test_sse_auth_completed_serialize() { + fn test_app_event_auth_completed_serialize() { let event = AppEvent::AuthCompleted { extension_name: "notion".to_string(), success: true, @@ -1072,14 +1055,14 @@ mod tests { } #[test] - fn test_ws_server_from_sse_auth_required() { - let sse = AppEvent::AuthRequired { + fn test_ws_server_from_app_event_auth_required() { + let event = AppEvent::AuthRequired { extension_name: "openai".to_string(), instructions: Some("Enter API key".to_string()), auth_url: None, setup_url: None, }; - let ws = WsServerMessage::from_app_event(&sse); + let ws = WsServerMessage::from_app_event(&event); match ws { WsServerMessage::Event { event_type, data } => { assert_eq!(event_type, "auth_required"); @@ -1090,13 +1073,13 @@ mod tests { } #[test] - fn test_ws_server_from_sse_auth_completed() { - let sse = AppEvent::AuthCompleted { + fn test_ws_server_from_app_event_auth_completed() { + let event = AppEvent::AuthCompleted { extension_name: "slack".to_string(), success: false, message: "Invalid token".to_string(), }; - let ws = WsServerMessage::from_app_event(&sse); + let ws = WsServerMessage::from_app_event(&event); match ws { WsServerMessage::Event { event_type, data } => { assert_eq!(event_type, "auth_completed"); diff --git a/src/channels/web/util.rs b/src/channels/web/util.rs index ed70c5ce..2e4ffe3b 100644 --- a/src/channels/web/util.rs +++ b/src/channels/web/util.rs @@ -4,6 +4,21 @@ use crate::channels::web::types::{ToolCallInfo, TurnInfo}; pub use ironclaw_common::truncate_preview; +/// Parse tool call summary JSON objects into `ToolCallInfo` structs. +fn parse_tool_call_infos(calls: &[serde_json::Value]) -> Vec { + calls + .iter() + .map(|c| ToolCallInfo { + name: c["name"].as_str().unwrap_or("unknown").to_string(), + has_result: c.get("result_preview").is_some_and(|v| !v.is_null()), + has_error: c.get("error").is_some_and(|v| !v.is_null()), + result_preview: c["result_preview"].as_str().map(String::from), + error: c["error"].as_str().map(String::from), + rationale: c["rationale"].as_str().map(String::from), + }) + .collect() +} + /// Build TurnInfo pairs from flat DB messages (user/tool_calls/assistant triples). /// /// Handles three message patterns: @@ -27,6 +42,7 @@ pub fn build_turns_from_db_messages( started_at: msg.created_at.to_rfc3339(), completed_at: None, tool_calls: Vec::new(), + narrative: None, }; // Check if next message is a tool_calls record @@ -34,18 +50,28 @@ pub fn build_turns_from_db_messages( && next.role == "tool_calls" { let tc_msg = iter.next().expect("peeked"); - match serde_json::from_str::>(&tc_msg.content) { - Ok(calls) => { - turn.tool_calls = calls - .iter() - .map(|c| ToolCallInfo { - name: c["name"].as_str().unwrap_or("unknown").to_string(), - has_result: c.get("result_preview").is_some(), - has_error: c.get("error").is_some(), - result_preview: c["result_preview"].as_str().map(String::from), - error: c["error"].as_str().map(String::from), - }) - .collect(); + // Parse tool_calls JSON — supports two formats: + // safety: no byte-index slicing; comment describes JSON shape + match serde_json::from_str::(&tc_msg.content) { + Ok(serde_json::Value::Array(calls)) => { + // Old format: plain array + turn.tool_calls = parse_tool_call_infos(&calls); + } + Ok(serde_json::Value::Object(obj)) => { + // New wrapped format with narrative + turn.narrative = obj + .get("narrative") + .and_then(|v| v.as_str()) + .map(String::from); + if let Some(serde_json::Value::Array(calls)) = obj.get("calls") { + turn.tool_calls = parse_tool_call_infos(calls); + } + } + Ok(_) => { + tracing::warn!( + message_id = %tc_msg.id, + "Unexpected tool_calls JSON shape in DB, skipping" + ); } Err(e) => { tracing::warn!( @@ -83,6 +109,7 @@ pub fn build_turns_from_db_messages( started_at: msg.created_at.to_rfc3339(), completed_at: Some(msg.created_at.to_rfc3339()), tool_calls: Vec::new(), + narrative: None, }); turn_number += 1; } @@ -201,4 +228,52 @@ mod tests { assert!(turns[0].tool_calls.is_empty()); assert_eq!(turns[0].state, "Completed"); } + + #[test] + fn test_build_turns_with_wrapped_tool_calls_format() { + let tc_json = serde_json::json!({ + "narrative": "Searching memory for context before proceeding.", + "calls": [ + {"name": "memory_search", "result_preview": "found 3 items", "rationale": "consult prior context"}, + {"name": "shell", "error": "permission denied"} + ] + }); + let messages = vec![ + make_msg("user", "Find info", 0), + make_msg("tool_calls", &tc_json.to_string(), 500), + make_msg("assistant", "Here's what I found", 1000), + ]; + let turns = build_turns_from_db_messages(&messages); + assert_eq!(turns.len(), 1); + assert_eq!( + turns[0].narrative.as_deref(), + Some("Searching memory for context before proceeding.") + ); + assert_eq!(turns[0].tool_calls.len(), 2); + assert_eq!(turns[0].tool_calls[0].name, "memory_search"); + assert_eq!( + turns[0].tool_calls[0].rationale.as_deref(), + Some("consult prior context") + ); + assert!(turns[0].tool_calls[0].has_result); + assert_eq!(turns[0].tool_calls[1].name, "shell"); + assert!(turns[0].tool_calls[1].has_error); + assert_eq!(turns[0].response.as_deref(), Some("Here's what I found")); + } + + #[test] + fn test_build_turns_wrapped_format_without_narrative() { + let tc_json = serde_json::json!({ + "calls": [{"name": "echo", "result_preview": "hello"}] + }); + let messages = vec![ + make_msg("user", "Say hi", 0), + make_msg("tool_calls", &tc_json.to_string(), 500), + make_msg("assistant", "Done", 1000), + ]; + let turns = build_turns_from_db_messages(&messages); + assert_eq!(turns.len(), 1); + assert!(turns[0].narrative.is_none()); + assert_eq!(turns[0].tool_calls.len(), 1); + } } diff --git a/src/channels/web/ws.rs b/src/channels/web/ws.rs index e209b4db..51beaafd 100644 --- a/src/channels/web/ws.rs +++ b/src/channels/web/ws.rs @@ -97,7 +97,7 @@ pub async fn handle_ws_connection( let msg = tokio::select! { event = event_stream.next() => { match event { - Some(sse_event) => WsServerMessage::from_app_event(&sse_event), + Some(app_event) => WsServerMessage::from_app_event(&app_event), None => break, // Broadcast channel closed } } @@ -520,7 +520,8 @@ mod tests { job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: "test".to_string(), + owner_id: "test".to_string(), + default_sender_id: "test".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: None, diff --git a/src/cli/oauth_defaults.rs b/src/cli/oauth_defaults.rs index 3b57872f..e9001909 100644 --- a/src/cli/oauth_defaults.rs +++ b/src/cli/oauth_defaults.rs @@ -62,6 +62,30 @@ pub fn builtin_client_id_override_env(secret_name: &str) -> Option<&'static str> } } +/// Suppress the baked-in desktop OAuth client secret when a hosted proxy is configured. +/// +/// In hosted deployments, IronClaw may resolve the platform Google client ID from +/// environment variables while still falling back to the baked-in desktop secret. +/// That client_id/client_secret mismatch breaks Google token exchange and refresh. +/// +/// When the proxy is configured, the platform will inject the correct server-side +/// secret for matching platform credentials, so the baked-in secret must be omitted. +pub fn hosted_proxy_client_secret( + client_secret: &Option, + builtin: Option<&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(), + } +} + // ── Shared callback server ────────────────────────────────────────────── // Core OAuth callback infrastructure is defined in `crate::llm::oauth_helpers` @@ -661,6 +685,48 @@ pub struct ProxyTokenExchangeRequest<'a> { pub extra_token_params: &'a HashMap, } +pub struct ProxyRefreshTokenRequest<'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 refresh_token: &'a str, + pub provider: Option<&'a str>, +} + +fn oauth_token_response_from_json( + token_data: serde_json::Value, + access_token_field: &str, +) -> Result { + let access_token = token_data + .get(access_token_field) + .and_then(|v| v.as_str()) + .ok_or_else(|| { + let fields: Vec<&str> = token_data + .as_object() + .map(|o| o.keys().map(|k| k.as_str()).collect()) + .unwrap_or_default(); + OAuthCallbackError::Io(format!( + "No '{}' field in proxy response (fields present: {:?})", + access_token_field, fields + )) + })? + .to_string(); + + let refresh_token = token_data + .get("refresh_token") + .and_then(|v| v.as_str()) + .map(String::from); + let expires_in = token_data.get("expires_in").and_then(|v| v.as_u64()); + + Ok(OAuthTokenResponse { + access_token, + refresh_token, + expires_in, + }) +} + /// Exchange an OAuth authorization code via the platform's token exchange proxy. /// /// Authenticated via the gateway auth token (Bearer header). The caller may @@ -682,6 +748,7 @@ pub async fn exchange_via_proxy( let client = reqwest::Client::builder() .timeout(Duration::from_secs(60)) + .redirect(reqwest::redirect::Policy::none()) .build() .map_err(|e| OAuthCallbackError::Io(format!("Failed to build HTTP client: {}", e)))?; let mut params = vec![ @@ -724,41 +791,350 @@ pub async fn exchange_via_proxy( .json() .await .map_err(|e| OAuthCallbackError::Io(format!("Failed to parse proxy response: {}", e)))?; + oauth_token_response_from_json(token_data, request.access_token_field) +} - let access_token = token_data - .get(request.access_token_field) - .and_then(|v| v.as_str()) - .ok_or_else(|| { - let fields: Vec<&str> = token_data - .as_object() - .map(|o| o.keys().map(|k| k.as_str()).collect()) - .unwrap_or_default(); - OAuthCallbackError::Io(format!( - "No '{}' field in proxy response (fields present: {:?})", - request.access_token_field, fields - )) - })? - .to_string(); +/// Refresh an OAuth access token via the platform's token refresh proxy. +/// +/// 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. +pub async fn refresh_token_via_proxy( + request: ProxyRefreshTokenRequest<'_>, +) -> Result { + if request.gateway_token.is_empty() { + return Err(OAuthCallbackError::Io( + "Gateway auth token is required for proxy token refresh".to_string(), + )); + } - let refresh_token = token_data - .get("refresh_token") - .and_then(|v| v.as_str()) - .map(String::from); - let expires_in = token_data.get("expires_in").and_then(|v| v.as_u64()); + let refresh_url = format!("{}/oauth/refresh", request.proxy_url.trim_end_matches('/')); + let client = reqwest::Client::builder() + .timeout(Duration::from_secs(15)) + .redirect(reqwest::redirect::Policy::none()) + .build() + .map_err(|e| OAuthCallbackError::Io(format!("Failed to build HTTP client: {}", e)))?; - Ok(OAuthTokenResponse { - access_token, - refresh_token, - expires_in, - }) + let mut params = vec![ + ("refresh_token", request.refresh_token.to_string()), + ("token_url", request.token_url.to_string()), + ("client_id", request.client_id.to_string()), + ]; + if let Some(secret) = request.client_secret { + params.push(("client_secret", secret.to_string())); + } + if let Some(provider) = request.provider { + params.push(("provider", provider.to_string())); + } + + let response = client + .post(&refresh_url) + .bearer_auth(request.gateway_token) + .form(¶ms) + .send() + .await + .map_err(|e| { + OAuthCallbackError::Io(format!("Token refresh proxy request failed: {}", e)) + })?; + + if !response.status().is_success() { + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + return Err(OAuthCallbackError::Io(format!( + "Token refresh proxy failed: {} - {}", + status, body + ))); + } + + let token_data: serde_json::Value = response + .json() + .await + .map_err(|e| OAuthCallbackError::Io(format!("Failed to parse proxy response: {}", e)))?; + + oauth_token_response_from_json(token_data, "access_token") } #[cfg(test)] mod tests { + use std::collections::HashMap; + use std::net::SocketAddr; + use std::sync::Arc; + + use axum::extract::{Form, State}; + use axum::http::HeaderMap; + use axum::response::Redirect; + use axum::routing::post; + use axum::{Json, Router}; + use serde_json::json; + use tokio::net::TcpListener; + use tokio::sync::{Mutex, oneshot}; + use crate::cli::oauth_defaults::{ builtin_credentials, callback_host, callback_url, is_loopback_host, landing_html, }; use crate::config::helpers::lock_env; + use crate::testing::credentials::{TEST_OAUTH_CLIENT_ID, TEST_OAUTH_CLIENT_SECRET}; + + #[derive(Clone, Debug, PartialEq, Eq)] + struct RecordedProxyRequest { + authorization: Option, + form: HashMap, + } + + #[derive(Clone)] + struct MockProxyState { + requests: Arc>>, + exchange_redirect_target: String, + refresh_redirect_target: String, + } + + struct MockProxyServer { + addr: SocketAddr, + requests: Arc>>, + shutdown_tx: Option>, + server_task: Option>, + } + + impl MockProxyServer { + async fn start() -> Self { + async fn exchange_handler( + State(state): State, + headers: HeaderMap, + Form(form): Form>, + ) -> Json { + state.requests.lock().await.push(RecordedProxyRequest { + authorization: headers + .get(axum::http::header::AUTHORIZATION) + .and_then(|value| value.to_str().ok()) + .map(str::to_string), + form, + }); + Json(json!({ + "access_token": "proxy-access-token", + "refresh_token": "proxy-refresh-token", + "expires_in": 7200 + })) + } + + async fn refresh_handler( + State(state): State, + headers: HeaderMap, + Form(form): Form>, + ) -> Json { + state.requests.lock().await.push(RecordedProxyRequest { + authorization: headers + .get(axum::http::header::AUTHORIZATION) + .and_then(|value| value.to_str().ok()) + .map(str::to_string), + form, + }); + Json(json!({ + "access_token": "proxy-access-token", + "refresh_token": "proxy-refresh-token", + "expires_in": 7200 + })) + } + + async fn exchange_redirect_handler(State(state): State) -> Redirect { + Redirect::temporary(&state.exchange_redirect_target) + } + + async fn refresh_redirect_handler(State(state): State) -> Redirect { + Redirect::temporary(&state.refresh_redirect_target) + } + + let requests = Arc::new(Mutex::new(Vec::new())); + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock proxy"); + let addr = listener.local_addr().expect("read mock proxy addr"); + let exchange_redirect_target = format!("http://{addr}/oauth/exchange"); + let refresh_redirect_target = format!("http://{addr}/oauth/refresh"); + let app = Router::new() + .route("/oauth/exchange", post(exchange_handler)) + .route("/oauth/refresh", post(refresh_handler)) + .route("/redirect/oauth/exchange", post(exchange_redirect_handler)) + .route("/redirect/oauth/refresh", post(refresh_redirect_handler)) + .with_state(MockProxyState { + requests: Arc::clone(&requests), + exchange_redirect_target, + refresh_redirect_target, + }); + let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>(); + let server_task = tokio::spawn(async move { + let _ = axum::serve(listener, app) + .with_graceful_shutdown(async { + let _ = shutdown_rx.await; + }) + .await; + }); + + Self { + addr, + requests, + shutdown_tx: Some(shutdown_tx), + server_task: Some(server_task), + } + } + + fn base_url(&self) -> String { + format!("http://{}", self.addr) + } + + fn redirecting_base_url(&self) -> String { + format!("{}/redirect", self.base_url()) + } + + async fn requests(&self) -> Vec { + self.requests.lock().await.clone() + } + + async fn shutdown(mut self) { + if let Some(tx) = self.shutdown_tx.take() { + let _ = tx.send(()); + } + if let Some(task) = self.server_task.take() { + let _ = task.await; + } + } + } + + impl Drop for MockProxyServer { + fn drop(&mut self) { + if let Some(tx) = self.shutdown_tx.take() { + let _ = tx.send(()); + } + if let Some(task) = self.server_task.take() { + task.abort(); + } + } + } + + #[test] + fn test_hosted_proxy_client_secret_suppresses_builtin_secret() { + let builtin = builtin_credentials("google_oauth_token").expect("google builtin creds"); + let client_secret = Some(builtin.client_secret.to_string()); + + let result = super::hosted_proxy_client_secret(&client_secret, Some(&builtin), true); + + assert_eq!(result, None); + } + + #[test] + fn test_hosted_proxy_client_secret_preserves_explicit_secret() { + let builtin = builtin_credentials("google_oauth_token").expect("google builtin creds"); + let client_secret = Some("hosted-server-secret".to_string()); + + let result = super::hosted_proxy_client_secret(&client_secret, Some(&builtin), true); + + assert_eq!(result, client_secret); + } + + #[tokio::test] + async fn test_refresh_token_via_proxy_sends_auth_and_form() { + let server = MockProxyServer::start().await; + + let response = super::refresh_token_via_proxy(super::ProxyRefreshTokenRequest { + proxy_url: &server.base_url(), + gateway_token: "gateway-test-token", + token_url: "https://oauth2.googleapis.com/token", + client_id: TEST_OAUTH_CLIENT_ID, + client_secret: Some(TEST_OAUTH_CLIENT_SECRET), + refresh_token: "refresh-token-123", + provider: Some("google"), + }) + .await + .expect("proxy refresh succeeds"); + + assert_eq!(response.access_token, "proxy-access-token"); + assert_eq!( + response.refresh_token.as_deref(), + Some("proxy-refresh-token") + ); + assert_eq!(response.expires_in, Some(7200)); + + let requests = server.requests().await; + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].authorization.as_deref(), + Some("Bearer gateway-test-token") + ); + assert_eq!( + requests[0].form.get("token_url").map(String::as_str), + Some("https://oauth2.googleapis.com/token") + ); + assert_eq!( + requests[0].form.get("client_id").map(String::as_str), + Some(TEST_OAUTH_CLIENT_ID) + ); + assert_eq!( + requests[0].form.get("client_secret").map(String::as_str), + Some(TEST_OAUTH_CLIENT_SECRET) + ); + assert_eq!( + requests[0].form.get("refresh_token").map(String::as_str), + Some("refresh-token-123") + ); + assert_eq!( + requests[0].form.get("provider").map(String::as_str), + Some("google") + ); + + server.shutdown().await; + } + + #[tokio::test] + async fn test_exchange_via_proxy_does_not_follow_redirects() { + let server = MockProxyServer::start().await; + + let error = match super::exchange_via_proxy(super::ProxyTokenExchangeRequest { + proxy_url: &server.redirecting_base_url(), + gateway_token: "gateway-test-token", + code: "auth-code-123", + redirect_uri: "http://localhost:3000/oauth/callback", + token_url: "https://oauth2.googleapis.com/token", + client_id: TEST_OAUTH_CLIENT_ID, + client_secret: Some(TEST_OAUTH_CLIENT_SECRET), + access_token_field: "access_token", + code_verifier: Some("code-verifier-123"), + extra_token_params: &HashMap::new(), + }) + .await + { + Ok(_) => panic!("redirected proxy exchange should fail"), + Err(error) => error, + }; + + assert!(error.to_string().contains("307")); + assert!(server.requests().await.is_empty()); + + server.shutdown().await; + } + + #[tokio::test] + async fn test_refresh_token_via_proxy_does_not_follow_redirects() { + let server = MockProxyServer::start().await; + + let error = match super::refresh_token_via_proxy(super::ProxyRefreshTokenRequest { + proxy_url: &server.redirecting_base_url(), + gateway_token: "gateway-test-token", + token_url: "https://oauth2.googleapis.com/token", + client_id: TEST_OAUTH_CLIENT_ID, + client_secret: Some(TEST_OAUTH_CLIENT_SECRET), + refresh_token: "refresh-token-123", + provider: Some("google"), + }) + .await + { + Ok(_) => panic!("redirected proxy refresh should fail"), + Err(error) => error, + }; + + assert!(error.to_string().contains("307")); + assert!(server.requests().await.is_empty()); + + server.shutdown().await; + } #[test] fn test_is_loopback_host() { diff --git a/src/cli/routines.rs b/src/cli/routines.rs index ebef8839..287663f6 100644 --- a/src/cli/routines.rs +++ b/src/cli/routines.rs @@ -10,7 +10,7 @@ use clap::Subcommand; use uuid::Uuid; use crate::agent::routine::{ - NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, next_cron_fire, + NotifyConfig, Routine, RoutineAction, RoutineGuardrails, RunStatus, Trigger, next_cron_fire, }; use crate::db::Database; @@ -251,15 +251,26 @@ async fn list( ); println!("{}", "-".repeat(130)); + // Fetch last-run status for all routines in a single batch query + let routine_ids: Vec = filtered.iter().map(|r| r.id).collect(); + let last_run_results = db + .batch_get_last_run_status(&routine_ids) + .await + .unwrap_or_default(); + for r in &filtered { - let status = if r.enabled { - if r.consecutive_failures > 0 { - format!("err({})", r.consecutive_failures) - } else { - "active".to_string() - } - } else { + let last_run_status = last_run_results.get(&r.id).copied(); + + let status = if !r.enabled { "disabled".to_string() + } else if last_run_status == Some(RunStatus::Running) { + "running".to_string() + } else if r.consecutive_failures > 0 { + format!("err({})", r.consecutive_failures) + } else if last_run_status == Some(RunStatus::Attention) { + "attention".to_string() + } else { + "active".to_string() }; let next_fire = r diff --git a/src/cli/tool.rs b/src/cli/tool.rs index be684580..9d39c492 100644 --- a/src/cli/tool.rs +++ b/src/cli/tool.rs @@ -2,6 +2,7 @@ //! //! Commands for installing, listing, removing, and authenticating WASM tools. +use std::collections::{HashMap, HashSet}; use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -79,6 +80,10 @@ pub enum ToolCommand { /// Directory to look for tool (default: ~/.ironclaw/tools/) #[arg(short, long)] dir: Option, + + /// User ID for checking credential status (default: "default") + #[arg(short, long, default_value = "default")] + user: String, }, /// Configure authentication for a tool @@ -124,7 +129,11 @@ pub async fn run_tool_command(cmd: ToolCommand) -> anyhow::Result<()> { } => install_tool(path, name, capabilities, target, release, skip_build, force).await, ToolCommand::List { dir, verbose } => list_tools(dir, verbose).await, ToolCommand::Remove { name, dir } => remove_tool(name, dir).await, - ToolCommand::Info { name_or_path, dir } => show_tool_info(name_or_path, dir).await, + ToolCommand::Info { + name_or_path, + dir, + user, + } => show_tool_info(name_or_path, dir, user).await, ToolCommand::Auth { name, dir, user } => auth_tool(name, dir, user).await, ToolCommand::Setup { name, dir, user } => setup_tool(name, dir, user).await, } @@ -388,7 +397,11 @@ async fn remove_tool(name: String, dir: Option) -> anyhow::Result<()> { } /// Show information about a tool. -async fn show_tool_info(name_or_path: String, dir: Option) -> anyhow::Result<()> { +async fn show_tool_info( + name_or_path: String, + dir: Option, + user_id: String, +) -> anyhow::Result<()> { let wasm_path = if name_or_path.ends_with(".wasm") { PathBuf::from(&name_or_path) } else { @@ -423,7 +436,37 @@ async fn show_tool_info(name_or_path: String, dir: Option) -> anyhow::R println!("\nCapabilities ({}):", caps_path.display()); let content = fs::read_to_string(&caps_path).await?; match CapabilitiesFile::from_json(&content) { - Ok(caps) => print_capabilities_detail(&caps), + Ok(caps) => { + // Lazily init secrets store only when auth secrets need checking. + let has_auth = caps.auth.is_some() + || caps + .setup + .as_ref() + .is_some_and(|s| !s.required_secrets.is_empty()) + || caps + .http + .as_ref() + .is_some_and(|h| !h.credentials.is_empty()); + let secrets_store = if has_auth { + match init_secrets_store().await { + Ok(store) => Some(store), + Err(e) => { + eprintln!(" Warning: could not init secrets store: {}", e); + None + } + } + } else { + None + }; + print_capabilities_detail( + &caps, + secrets_store + .as_ref() + .map(|s| s.as_ref() as &(dyn SecretsStore + Send + Sync)), + &user_id, + ) + .await; + } Err(e) => println!(" Error parsing: {}", e), } } else { @@ -476,8 +519,89 @@ fn print_capabilities_summary(caps: &CapabilitiesFile) { } } +/// Per-secret info collected from all auth-related capability sections. +struct AuthSecretInfo { + secret_name: String, + /// Human-readable label (from auth.display_name or setup prompt). + description: Option, + /// Injection location (from http.credentials). + location: Option, +} + +/// Collected auth secrets and the set of secret names they cover. +struct CollectedAuthSecrets { + secrets: Vec, + /// Secret names present in `secrets`, for filtering the Secrets capability section. + seen_names: HashSet, +} + +/// Collect and deduplicate auth secrets from all auth-related capability sections. +/// +/// Priority for the description label: auth.display_name > setup.required_secrets.prompt. +/// Injection location is merged from http.credentials. +fn collect_auth_secrets(caps: &CapabilitiesFile) -> CollectedAuthSecrets { + let mut secrets: Vec = Vec::new(); + let mut seen: HashMap = HashMap::new(); + + // auth.display_name is the best label — seed first. + if let Some(ref auth) = caps.auth { + let index = secrets.len(); + seen.insert(auth.secret_name.clone(), index); + secrets.push(AuthSecretInfo { + secret_name: auth.secret_name.clone(), + description: auth.display_name.clone(), + location: None, + }); + } + + // setup.required_secrets.prompt is second-best label. + if let Some(ref setup) = caps.setup { + for secret in &setup.required_secrets { + if !seen.contains_key(&secret.name) { + let index = secrets.len(); + seen.insert(secret.name.clone(), index); + secrets.push(AuthSecretInfo { + secret_name: secret.name.clone(), + description: Some(secret.prompt.clone()), + location: None, + }); + } + } + } + + // Merge injection location from http.credentials. + if let Some(ref http) = caps.http { + for cred in http.credentials.values() { + let loc = format!("{:?}", cred.location); + if let Some(&index) = seen.get(&cred.secret_name) { + secrets[index].location = Some(loc); + } else { + let index = secrets.len(); + seen.insert(cred.secret_name.clone(), index); + secrets.push(AuthSecretInfo { + secret_name: cred.secret_name.clone(), + description: None, + location: Some(loc), + }); + } + } + } + + let seen_names = seen.into_keys().collect(); + CollectedAuthSecrets { + secrets, + seen_names, + } +} + /// Print detailed capabilities. -fn print_capabilities_detail(caps: &CapabilitiesFile) { +async fn print_capabilities_detail( + caps: &CapabilitiesFile, + secrets_store: Option<&(dyn SecretsStore + Send + Sync)>, + user_id: &str, +) { + let mut collected = collect_auth_secrets(caps); + if let Some(ref http) = caps.http { println!(" HTTP:"); for endpoint in &http.allowlist { @@ -490,13 +614,6 @@ fn print_capabilities_detail(caps: &CapabilitiesFile) { println!(" {} {} {}", methods, endpoint.host, path); } - if !http.credentials.is_empty() { - println!(" Credentials:"); - for (key, cred) in &http.credentials { - println!(" {}: {} -> {:?}", key, cred.secret_name, cred.location); - } - } - if let Some(ref rate) = http.rate_limit { println!( " Rate limit: {}/min, {}/hour", @@ -505,12 +622,24 @@ fn print_capabilities_detail(caps: &CapabilitiesFile) { } } + // Filter secrets already covered by the auth section (always rendered when non-empty). if let Some(ref secrets) = caps.secrets && !secrets.allowed_names.is_empty() { - println!(" Secrets (existence check only):"); - for name in &secrets.allowed_names { - println!(" {}", name); + let extra: Vec<_> = if collected.secrets.is_empty() { + secrets.allowed_names.iter().collect() + } else { + secrets + .allowed_names + .iter() + .filter(|name| !collected.seen_names.contains(name.as_str())) + .collect() + }; + if !extra.is_empty() { + println!(" Secrets (existence check only):"); + for name in extra { + println!(" {}", name); + } } } @@ -531,6 +660,38 @@ fn print_capabilities_detail(caps: &CapabilitiesFile) { println!(" {}", prefix); } } + + // Consolidated auth status — sorted by secret name for deterministic output. + if !collected.secrets.is_empty() { + collected + .secrets + .sort_by(|a, b| a.secret_name.cmp(&b.secret_name)); + println!(" Auth:"); + for info in &collected.secrets { + let (icon, label) = match secrets_store { + Some(store) => match store.exists(user_id, &info.secret_name).await { + Ok(true) => ("\u{2713}", "configured"), + Ok(false) => ("\u{2717}", "missing"), + Err(e) => { + eprintln!( + " Warning: failed to check secret `{}`: {}", + info.secret_name, e + ); + ("?", "unknown") + } + }, + None => ("?", "unknown"), + }; + let mut parts = info.secret_name.clone(); + if let Some(ref desc) = info.description { + parts = format!("{} ({})", parts, desc); + } + if let Some(ref loc) = info.location { + parts = format!("{} -> {}", parts, loc); + } + println!(" {} {} {}", parts, icon, label); + } + } } /// Validate a tool name to prevent path traversal. @@ -677,8 +838,7 @@ async fn combine_provider_scopes( secret_name: &str, base_oauth: &crate::tools::wasm::OAuthConfigSchema, ) -> crate::tools::wasm::OAuthConfigSchema { - let mut all_scopes: std::collections::HashSet = - base_oauth.scopes.iter().cloned().collect(); + let mut all_scopes: HashSet = base_oauth.scopes.iter().cloned().collect(); if let Ok(mut entries) = tokio::fs::read_dir(tools_dir).await { while let Ok(Some(entry)) = entries.next_entry().await { @@ -1127,6 +1287,8 @@ async fn setup_tool(name: String, dir: Option, user_id: String) -> anyh #[cfg(test)] mod tests { use super::*; + use crate::secrets::{CreateSecretParams, SecretsStore}; + use crate::testing::credentials::test_secrets_store; #[test] fn test_format_size() { @@ -1143,4 +1305,96 @@ mod tests { assert!(dir.to_string_lossy().contains(".ironclaw")); assert!(dir.to_string_lossy().contains("tools")); } + + /// Verify that auth secrets are deduplicated across auth, setup, and http.credentials, + /// and that credential status is checked against the secrets store. + #[tokio::test] + async fn test_auth_secret_dedup_and_status() { + let caps = CapabilitiesFile::from_json( + r#"{ + "auth": { + "secret_name": "gh_token", + "display_name": "GitHub" + }, + "setup": { + "required_secrets": [ + { "name": "gh_token", "prompt": "GitHub PAT" }, + { "name": "extra_key", "prompt": "Extra API Key" } + ] + }, + "http": { + "allowlist": [{ "host": "api.github.com" }], + "credentials": { + "github": { + "secret_name": "gh_token", + "location": { "type": "bearer" }, + "host_patterns": ["api.github.com"] + } + } + }, + "secrets": { + "allowed_names": ["gh_token", "gh_*"] + } + }"#, + ) + .unwrap(); + + let collected = collect_auth_secrets(&caps); + + // gh_token should appear once (from auth), with location merged from credentials. + // extra_key should appear once (from setup). + assert_eq!(collected.secrets.len(), 2); + let gh = collected + .secrets + .iter() + .find(|s| s.secret_name == "gh_token") + .unwrap(); + assert_eq!(gh.description.as_deref(), Some("GitHub")); + assert!( + gh.location.is_some(), + "location should be merged from http.credentials" + ); + + let extra = collected + .secrets + .iter() + .find(|s| s.secret_name == "extra_key") + .unwrap(); + assert_eq!(extra.description.as_deref(), Some("Extra API Key")); + assert!(extra.location.is_none()); + + // Secrets section should filter gh_token (in seen_names) but keep gh_* (wildcard). + let secrets = caps.secrets.as_ref().unwrap(); + let extra_secrets: Vec<_> = secrets + .allowed_names + .iter() + .filter(|name| !collected.seen_names.contains(name.as_str())) + .collect(); + assert_eq!(extra_secrets, vec!["gh_*"]); + + // Verify store check: missing secret -> exists returns false. + let store = test_secrets_store(); + assert!(!store.exists("default", "gh_token").await.unwrap()); + + // Store gh_token and verify it's found. + store + .create( + "default", + CreateSecretParams::new("gh_token", "ghp_test123"), + ) + .await + .unwrap(); + assert!(store.exists("default", "gh_token").await.unwrap()); + // extra_key still missing. + assert!(!store.exists("default", "extra_key").await.unwrap()); + } + + /// No auth sections → collect_auth_secrets returns empty. + #[test] + fn test_collect_auth_secrets_empty_caps() { + let caps = CapabilitiesFile::default(); + let collected = collect_auth_secrets(&caps); + assert!(collected.secrets.is_empty()); + assert!(collected.seen_names.is_empty()); + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index dcda0fe9..a362fd09 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -312,13 +312,11 @@ impl Config { let tunnel = TunnelConfig::resolve(settings)?; let channels = ChannelsConfig::resolve(settings, &owner_id)?; - // Resolve workspace config using the gateway user_id for default layers. - let workspace_user_id = channels - .gateway - .as_ref() - .map(|gw| gw.user_id.as_str()) - .unwrap_or("default"); - let workspace = WorkspaceConfig::resolve(workspace_user_id)?; + // Resolve the startup workspace against the durable owner scope. The + // gateway may expose a distinct sender identity, but the base runtime + // workspace stays owner-scoped and per-user gateway workspaces are + // handled separately by WorkspacePool. + let workspace = WorkspaceConfig::resolve(&owner_id)?; Ok(Self { owner_id: owner_id.clone(), diff --git a/src/db/libsql/routines.rs b/src/db/libsql/routines.rs index 6702cc1b..69c9f5c0 100644 --- a/src/db/libsql/routines.rs +++ b/src/db/libsql/routines.rs @@ -462,6 +462,56 @@ impl RoutineStore for LibSqlBackend { Ok(counts) } + async fn batch_get_last_run_status( + &self, + routine_ids: &[Uuid], + ) -> Result, DatabaseError> { + if routine_ids.is_empty() { + return Ok(HashMap::new()); + } + + let conn = self.connect().await?; + + // SQLite doesn't support ANY($1), so we query all latest runs and filter in memory. + // Uses a subquery to pick only the most recent run per routine. + let mut rows = conn + .query( + "SELECT routine_id, status FROM routine_runs r1 + WHERE started_at = ( + SELECT MAX(started_at) FROM routine_runs r2 + WHERE r2.routine_id = r1.routine_id + ) + GROUP BY routine_id", + params![], + ) + .await + .map_err(|e| { + DatabaseError::Query(format!("Failed to batch get last run status: {}", e)) + })?; + + let routine_id_set: HashSet = routine_ids.iter().copied().collect(); + let mut statuses = HashMap::new(); + + while let Some(row) = rows + .next() + .await + .map_err(|e| DatabaseError::Query(e.to_string()))? + { + let id_str: String = get_text(&row, 0); + let id = Uuid::parse_str(&id_str) + .map_err(|e| DatabaseError::Query(format!("Invalid routine UUID: {}", e)))?; + + if routine_id_set.contains(&id) { + let status_str: String = get_text(&row, 1); + if let std::result::Result::Ok(status) = status_str.parse::() { + statuses.insert(id, status); + } + } + } + + Ok(statuses) + } + async fn link_routine_run_to_job( &self, run_id: Uuid, diff --git a/src/db/mod.rs b/src/db/mod.rs index c0594bda..6d984fed 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -528,6 +528,15 @@ pub trait RoutineStore: Send + Sync { &self, routine_ids: &[Uuid], ) -> Result, DatabaseError>; + + /// Fetch the last run status for multiple routines in a single query. + /// Returns a map from routine_id to its most recent RunStatus. + /// Routines with no runs are omitted from the result. + async fn batch_get_last_run_status( + &self, + routine_ids: &[Uuid], + ) -> Result, DatabaseError>; + async fn link_routine_run_to_job( &self, run_id: Uuid, diff --git a/src/db/postgres.rs b/src/db/postgres.rs index a2c686d3..7bf76001 100644 --- a/src/db/postgres.rs +++ b/src/db/postgres.rs @@ -510,6 +510,14 @@ impl RoutineStore for PgBackend { .await } + async fn batch_get_last_run_status( + &self, + routine_ids: &[Uuid], + ) -> Result, DatabaseError> + { + self.store.batch_get_last_run_status(routine_ids).await + } + async fn link_routine_run_to_job( &self, run_id: Uuid, diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index b53cc257..90920767 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -53,22 +53,6 @@ struct HostedOAuthFlowStart { 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() { @@ -891,24 +875,27 @@ impl ExtensionManager { *self.relay_channel_manager.write().await = Some(channel_manager); } - /// Check if a channel name corresponds to a relay extension (has stored stream token + /// Check if a channel name corresponds to a relay extension (has stored team_id /// or is tracked in the installed relay extensions set). pub async fn is_relay_channel(&self, name: &str, user_id: &str) -> bool { // Check in-memory installed set first (supports no-store mode) if self.installed_relay_extensions.read().await.contains(name) { return true; } - // Then check for stored stream token - self.secrets - .exists(user_id, &format!("relay:{}:stream_token", name)) - .await - .unwrap_or(false) + // Check for stored team_id (persisted across restarts by the OAuth callback) + if let Some(ref store) = self.store { + let key = format!("relay:{}:team_id", name); + if let Ok(Some(v)) = store.get_setting(user_id, &key).await { + return v.as_str().is_some_and(|s| !s.is_empty()); + } + } + false } /// Restore persisted relay channels after startup. /// /// Loads the persisted active channel list, filters to relay types (those with - /// a stored stream token), and activates each via `activate_stored_relay()`. + /// a stored team_id setting), and activates each via `activate_stored_relay()`. /// Skips channels that are already active. /// /// Call this only after `set_relay_channel_manager()` or `set_channel_runtime()`. @@ -1428,9 +1415,11 @@ impl ExtensionManager { if kind_filter.is_none() || kind_filter == Some(ExtensionKind::ChannelRelay) { let installed = self.installed_relay_extensions.read().await; let active_names = self.active_channel_names.read().await; + let errors = self.activation_errors.read().await; for name in installed.iter() { let active = active_names.contains(name); - let has_token = self.is_relay_channel(name, user_id).await; + let authenticated = self.is_relay_channel(name, user_id).await; + let activation_error = errors.get(name).cloned(); let registry_entry = self .registry .get_with_kind(name, Some(ExtensionKind::ChannelRelay)) @@ -1443,13 +1432,13 @@ impl ExtensionManager { display_name, description, url: None, - authenticated: has_token, + authenticated, active, tools: Vec::new(), needs_setup: false, has_auth: true, installed: true, - activation_error: None, + activation_error, version: None, }); } @@ -1626,7 +1615,22 @@ impl ExtensionManager { self.persist_active_channels(user_id).await; self.activation_errors.write().await.remove(name); - // Remove stored stream token + // Remove stored team_id setting and clean up secrets + if let Some(ref store) = self.store + && let Err(e) = store + .delete_setting(user_id, &format!("relay:{}:team_id", name)) + .await + { + tracing::warn!(error = %e, name, "Failed to delete relay team_id setting on removal"); + } + if let Err(e) = self + .secrets + .delete(user_id, &format!("relay:{}:oauth_state", name)) + .await + { + tracing::warn!(error = %e, name, "Failed to delete relay oauth_state secret on removal"); + } + // Clean up legacy stream_token secret from pre-webhook installs let _ = self .secrets .delete(user_id, &format!("relay:{}:stream_token", name)) @@ -3179,7 +3183,7 @@ impl ExtensionManager { // 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( + let proxy_client_secret = oauth_defaults::hosted_proxy_client_secret( &client_secret, builtin.as_ref(), oauth_defaults::exchange_proxy_url().is_some(), @@ -3284,7 +3288,7 @@ impl ExtensionManager { } .await; - // Broadcast SSE event + // Broadcast auth result event let (success, message) = match result { Ok(()) => (true, format!("{} authenticated successfully", display_name)), Err(ref e) => ( @@ -4181,13 +4185,13 @@ impl ExtensionManager { /// /// For Slack: initiates OAuth flow (redirect-based). /// For Telegram: accepts a bot token, registers it with channel-relay, - /// and stores the returned stream token. + /// and stores the team_id setting. async fn auth_channel_relay( &self, name: &str, user_id: &str, ) -> Result { - // Check if already authenticated (stream token exists) + // Check if already authenticated (team_id setting exists) if self.is_relay_channel(name, user_id).await { return Ok(AuthResult::authenticated(name, ExtensionKind::ChannelRelay)); } @@ -4233,19 +4237,9 @@ impl ExtensionManager { name: &str, user_id: &str, ) -> Result { - let token_key = format!("relay:{}:stream_token", name); let team_id_key = format!("relay:{}:team_id", name); - // Check if we have a stream token - // Verify auth: stream token must exist (even though we don't use it in this constructor path) - let _stream_token = match self.secrets.get_decrypted(user_id, &token_key).await { - Ok(secret) => secret.expose().to_string(), - Err(_) => { - return Err(ExtensionError::AuthRequired); - } - }; - - // Get team_id from settings + // Get team_id from settings (stored by the OAuth callback) let team_id = if let Some(ref store) = self.store { store .get_setting(user_id, &team_id_key) @@ -4258,6 +4252,10 @@ impl ExtensionManager { String::new() }; + if team_id.is_empty() { + return Err(ExtensionError::AuthRequired); + } + // Use relay config captured at startup let relay_config = self.relay_config()?; @@ -4367,11 +4365,11 @@ impl ExtensionManager { return Ok(ExtensionKind::WasmChannel); } - // Check channel-relay extensions (installed in memory or has stored token) + // Check channel-relay extensions (installed in memory or has stored team_id) if self.installed_relay_extensions.read().await.contains(name) { return Ok(ExtensionKind::ChannelRelay); } - // Also check if there's a stored stream token (persisted across restarts) + // Also check if there's a stored team_id setting (persisted across restarts) if self.is_relay_channel(name, user_id).await { return Ok(ExtensionKind::ChannelRelay); } @@ -4999,11 +4997,7 @@ impl ExtensionManager { names.insert(server.token_secret_name()); (names, Vec::new()) } - ExtensionKind::ChannelRelay => { - let mut names = std::collections::HashSet::new(); - names.insert(format!("relay:{}:stream_token", name)); - (names, Vec::new()) - } + ExtensionKind::ChannelRelay => (std::collections::HashSet::new(), Vec::new()), }; let allowed_fields: std::collections::HashSet = @@ -5434,7 +5428,9 @@ impl ExtensionManager { .map_err(|e| ExtensionError::NotInstalled(e.to_string()))?; server.token_secret_name() } - ExtensionKind::ChannelRelay => format!("relay:{}:stream_token", name), + ExtensionKind::ChannelRelay => { + return Err(ExtensionError::AuthRequired); + } }; let mut secrets = std::collections::HashMap::new(); @@ -5702,7 +5698,7 @@ mod tests { use crate::extensions::manager::{ ChannelRuntimeState, FallbackDecision, TelegramBindingData, TelegramBindingResult, TelegramOwnerBindingState, build_wasm_channel_runtime_config_updates, - combine_install_errors, fallback_decision, hosted_proxy_client_secret, infer_kind_from_url, + combine_install_errors, fallback_decision, infer_kind_from_url, normalize_hosted_callback_url, send_telegram_text_message, telegram_message_matches_verification_code, }; @@ -7043,7 +7039,7 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); let mgr = make_test_manager(None, dir.path().to_path_buf()); - // No token stored → not a relay channel + // No store configured, no team_id → not a relay channel assert!(!mgr.is_relay_channel("slack-relay", "test").await); } @@ -7862,19 +7858,13 @@ mod tests { .await .insert("test-relay".to_string()); - // configure() should dispatch to activate_channel_relay(), not - // activate_wasm_channel(). Both will fail (no runtime configured), - // but the error should be about relay config, not WASM channels. - let mut secrets = std::collections::HashMap::new(); - secrets.insert( - "relay:test-relay:stream_token".to_string(), - "tok".to_string(), - ); - + // configure() with empty secrets should dispatch to + // activate_channel_relay(), not activate_wasm_channel(). Relay auth + // is OAuth-only so there are no manual secrets to pass. let result = mgr .configure( "test-relay", - &secrets, + &std::collections::HashMap::new(), &std::collections::HashMap::new(), "test", ) @@ -7886,7 +7876,6 @@ mod tests { ); let result = result.unwrap(); - // Activation will fail (no relay config), but secrets should still be stored assert!( !result.activated, "activation should fail without relay config" @@ -7896,15 +7885,6 @@ mod tests { "error should not mention WASM — got: {}", result.message ); - - // Verify the secret was stored - assert!( - mgr.secrets - .exists("test", "relay:test-relay:stream_token") - .await - .unwrap_or(false), - "configure should have stored the relay stream token" - ); } #[test] fn test_validation_failed_is_distinct_error_variant() { @@ -7970,7 +7950,8 @@ mod tests { 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); + let result = + crate::cli::oauth_defaults::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" @@ -7982,7 +7963,8 @@ mod tests { 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); + let result = + crate::cli::oauth_defaults::hosted_proxy_client_secret(&secret, builtin.as_ref(), true); assert_eq!( result, Some("user-entered-custom-secret".to_string()), @@ -7996,7 +7978,8 @@ mod tests { 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); + let result = + crate::cli::oauth_defaults::hosted_proxy_client_secret(&secret, builtin_ref, false); assert_eq!( result, secret, "built-in secret must be kept when the callback will exchange directly" @@ -8007,7 +7990,8 @@ mod tests { 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); + let result = + crate::cli::oauth_defaults::hosted_proxy_client_secret(&None, builtin.as_ref(), true); assert_eq!( result, None, "None secret stays None even when the exchange proxy is configured" @@ -8021,7 +8005,8 @@ mod tests { assert!(builtin.is_none()); let secret = Some("dcr-secret".to_string()); - let result = hosted_proxy_client_secret(&secret, builtin.as_ref(), true); + let result = + crate::cli::oauth_defaults::hosted_proxy_client_secret(&secret, builtin.as_ref(), true); assert_eq!( result, Some("dcr-secret".to_string()), diff --git a/src/history/store.rs b/src/history/store.rs index d6570b3c..1e4cdd82 100644 --- a/src/history/store.rs +++ b/src/history/store.rs @@ -1403,6 +1403,40 @@ impl Store { Ok(counts) } + /// Batch-load the most recent run status for multiple routines in a single query. + /// Uses a window function to pick only the latest run per routine. + #[cfg(feature = "postgres")] + pub async fn batch_get_last_run_status( + &self, + routine_ids: &[Uuid], + ) -> Result, DatabaseError> { + if routine_ids.is_empty() { + return Ok(HashMap::new()); + } + + let conn = self.conn().await?; + let rows = conn + .query( + "SELECT DISTINCT ON (routine_id) routine_id, status + FROM routine_runs + WHERE routine_id = ANY($1) + ORDER BY routine_id, started_at DESC", + &[&routine_ids], + ) + .await?; + + let mut statuses = HashMap::new(); + for row in rows { + let id: Uuid = row.get("routine_id"); + let status_str: String = row.get("status"); + if let std::result::Result::Ok(status) = status_str.parse::() { + statuses.insert(id, status); + } + } + + Ok(statuses) + } + /// Link a routine run to a dispatched job. pub async fn link_routine_run_to_job( &self, diff --git a/src/llm/anthropic_oauth.rs b/src/llm/anthropic_oauth.rs index 490fbc3f..c94c90e5 100644 --- a/src/llm/anthropic_oauth.rs +++ b/src/llm/anthropic_oauth.rs @@ -575,6 +575,7 @@ fn extract_response_content(response: &AnthropicResponse) -> (Option, Ve id: id.clone(), name: name.clone(), arguments: input.clone(), + reasoning: None, }); } } @@ -623,6 +624,7 @@ mod tests { id: "call_1".to_string(), name: "search".to_string(), arguments: serde_json::json!({"q": "test"}), + reasoning: None, }]; let messages = vec![ ChatMessage::user("Search for test"), diff --git a/src/llm/bedrock.rs b/src/llm/bedrock.rs index 5d6e121e..b5f7badd 100644 --- a/src/llm/bedrock.rs +++ b/src/llm/bedrock.rs @@ -522,6 +522,7 @@ fn extract_content_blocks( id: tu.tool_use_id().to_string(), name: tu.name().to_string(), arguments: document_to_json(tu.input()), + reasoning: None, }); } // Ignore reasoning, citations, images, etc. @@ -759,11 +760,13 @@ mod tests { id: "call_1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({"text": "hi"}), + reasoning: None, }; let tc2 = crate::llm::provider::ToolCall { id: "call_2".to_string(), name: "time".to_string(), arguments: serde_json::json!({}), + reasoning: None, }; let messages = vec![ @@ -802,6 +805,7 @@ mod tests { id: "call_1".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }; let messages = vec![ @@ -825,6 +829,7 @@ mod tests { id: "call_1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({}), + reasoning: None, }; let messages = vec![ @@ -989,11 +994,13 @@ mod tests { id: "call_abc".to_string(), name: "get_weather".to_string(), arguments: serde_json::json!({"city": "NYC"}), + reasoning: None, }; let tc2 = crate::llm::provider::ToolCall { id: "call_def".to_string(), name: "get_time".to_string(), arguments: serde_json::json!({"tz": "EST"}), + reasoning: None, }; let messages = vec![ diff --git a/src/llm/codex_chatgpt.rs b/src/llm/codex_chatgpt.rs index 56cb3378..e7dcf40d 100644 --- a/src/llm/codex_chatgpt.rs +++ b/src/llm/codex_chatgpt.rs @@ -732,6 +732,7 @@ impl LlmProvider for CodexChatGptProvider { id: tc.call_id, name: tc.name, arguments: args, + reasoning: None, } }) .collect(); @@ -825,6 +826,7 @@ mod tests { id: "call_1".to_string(), name: "search".to_string(), arguments: json!({"query": "rust"}), + reasoning: None, }; let msg = ChatMessage::assistant_with_tool_calls(Some("thinking...".into()), vec![tc]); let items = CodexChatGptProvider::message_to_input_items(&msg); diff --git a/src/llm/gemini_oauth.rs b/src/llm/gemini_oauth.rs index b36eb595..a19eec12 100644 --- a/src/llm/gemini_oauth.rs +++ b/src/llm/gemini_oauth.rs @@ -1898,6 +1898,7 @@ impl GeminiOauthProvider { id, name, arguments: args, + reasoning: None, }); } } diff --git a/src/llm/github_copilot.rs b/src/llm/github_copilot.rs index b173191a..c7a24b1a 100644 --- a/src/llm/github_copilot.rs +++ b/src/llm/github_copilot.rs @@ -596,6 +596,7 @@ fn extract_choice_content(choice: &OpenAiChoice) -> (Option, Vec = - raw_messages.into_iter().map(|m| m.into()).collect(); + let raw: Vec = raw_messages.into_iter().map(|m| m.into()).collect(); + + // NEAR AI rejects `role:"tool"` messages even on text-only completion paths. + // Apply the same flattening used by complete_with_tools(). + let messages = if self.flatten_tool_messages { + flatten_tool_messages(raw) + } else { + raw + }; let request = ChatCompletionRequest { model, @@ -580,6 +587,7 @@ impl LlmProvider for NearAiChatProvider { id: tc.id, name: tc.function.name, arguments, + reasoning: None, } }) .collect(); @@ -1173,11 +1181,13 @@ mod tests { id: "call_1".to_string(), name: "list_issues".to_string(), arguments: serde_json::json!({"owner": "foo", "repo": "bar"}), + reasoning: None, }, ToolCall { id: "call_2".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }, ]; @@ -1210,6 +1220,7 @@ mod tests { id: "call_1".to_string(), name: "test".to_string(), arguments: serde_json::json!({"key": "value"}), + reasoning: None, }; let msg = ChatMessage::assistant_with_tool_calls(None, vec![tc]); let chat_msg: ChatCompletionMessage = msg.into(); @@ -1453,6 +1464,7 @@ mod tests { id: tc.id, name: tc.function.name, arguments, + reasoning: None, } }) .collect(); @@ -1502,6 +1514,7 @@ mod tests { id: tc.id, name: tc.function.name, arguments, + reasoning: None, } }) .collect(); @@ -2124,6 +2137,7 @@ mod tests { id: "call_1".to_string(), name: "test".to_string(), arguments: serde_json::json!({}), + reasoning: None, }], ); let chat_msg: ChatCompletionMessage = msg.into(); @@ -2193,6 +2207,65 @@ mod tests { assert_eq!(deserialized.function.arguments, r#"{"city":"London"}"#); } + // -- flatten_tool_messages in complete() path ---------------------------- + + #[test] + fn test_flatten_applied_on_text_only_path() { + // Verify that flatten_tool_messages converts tool-role messages to user + // messages (mirrors the complete_with_tools path). + let messages = vec![ + ChatCompletionMessage { + role: "user".to_string(), + content: Some(MessageContent::Text("run it".to_string())), + tool_call_id: None, + name: None, + tool_calls: None, + }, + ChatCompletionMessage { + role: "tool".to_string(), + content: Some(MessageContent::Text("ok".to_string())), + tool_call_id: Some("call_1".to_string()), + name: Some("run_cmd".to_string()), + tool_calls: None, + }, + ]; + let flattened = flatten_tool_messages(messages); + assert_eq!(flattened.len(), 2); + assert_eq!(flattened[1].role, "user"); + let text = flattened[1] + .content + .as_ref() + .and_then(|c| c.as_text()) + .unwrap(); + assert!(text.contains("run_cmd"), "should reference tool name"); + assert!(text.contains("ok"), "should include tool result"); + } + + #[test] + fn test_no_flatten_when_no_tool_messages() { + // When there are no tool-role messages, flatten_tool_messages is a no-op. + let messages = vec![ + ChatCompletionMessage { + role: "user".to_string(), + content: Some(MessageContent::Text("hi".to_string())), + tool_call_id: None, + name: None, + tool_calls: None, + }, + ChatCompletionMessage { + role: "assistant".to_string(), + content: Some(MessageContent::Text("hello".to_string())), + tool_call_id: None, + name: None, + tool_calls: None, + }, + ]; + let result = flatten_tool_messages(messages); + // No tool messages → unchanged roles + assert_eq!(result[0].role, "user"); + assert_eq!(result[1].role, "assistant"); + } + // -- api_url edge cases --------------------------------------------------- #[test] diff --git a/src/llm/openai_codex_provider.rs b/src/llm/openai_codex_provider.rs index 9e3aa955..3449a08a 100644 --- a/src/llm/openai_codex_provider.rs +++ b/src/llm/openai_codex_provider.rs @@ -625,6 +625,7 @@ fn parse_sse_response(body: &str) -> Result { id: state.call_id, name: state.name, arguments, + reasoning: None, }); } else { // Fallback: extract directly from the item @@ -650,6 +651,7 @@ fn parse_sse_response(body: &str) -> Result { id: call_id, name, arguments, + reasoning: None, }); } } @@ -727,6 +729,7 @@ fn parse_sse_response(body: &str) -> Result { id: state.call_id, name: state.name, arguments, + reasoning: None, }); } } @@ -822,11 +825,13 @@ mod tests { id: "call_1".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }, ToolCall { id: "call_2".to_string(), name: "read".to_string(), arguments: serde_json::json!({"path": "/tmp"}), + reasoning: None, }, ]; let msg = diff --git a/src/llm/provider.rs b/src/llm/provider.rs index bb45ec68..8afd914a 100644 --- a/src/llm/provider.rs +++ b/src/llm/provider.rs @@ -231,6 +231,10 @@ pub struct ToolCall { pub id: String, pub name: String, pub arguments: serde_json::Value, + /// Optional reasoning for why this tool was chosen — supplied by the provider + /// or derived from the shared response content as a fallback. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reasoning: Option, } /// Generate a tool-call ID that satisfies all providers. @@ -637,6 +641,7 @@ mod tests { id: "call_1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({}), + reasoning: None, }; let mut messages = vec![ ChatMessage::user("hello"), @@ -680,6 +685,7 @@ mod tests { id: "call_1".to_string(), name: "echo".to_string(), arguments: serde_json::json!({}), + reasoning: None, }; let mut messages = vec![ ChatMessage::user("test"), @@ -705,11 +711,13 @@ mod tests { id: "call_sel_1".to_string(), name: "search".to_string(), arguments: serde_json::json!({"q": "test"}), + reasoning: None, }; let tc2 = ToolCall { id: "call_sel_2".to_string(), name: "http".to_string(), arguments: serde_json::json!({"url": "https://example.com"}), + reasoning: None, }; let mut messages = vec![ ChatMessage::system("You are a helpful assistant."), diff --git a/src/llm/reasoning.rs b/src/llm/reasoning.rs index cbec297b..77905f95 100644 --- a/src/llm/reasoning.rs +++ b/src/llm/reasoning.rs @@ -525,17 +525,35 @@ impl Reasoning { let response = self.llm.complete_with_tools(request).await?; - let reasoning = response.content.unwrap_or_default(); + let shared_reasoning = response + .content + .map(|c| { + let pre_truncated = truncate_at_tool_tags(&c); + clean_response(&pre_truncated) + }) + .unwrap_or_default(); let selections: Vec = response .tool_calls .into_iter() - .map(|tool_call| ToolSelection { - tool_name: tool_call.name, - parameters: tool_call.arguments, - reasoning: reasoning.clone(), - alternatives: vec![], - tool_call_id: tool_call.id, + .map(|tool_call| { + // Prefer per-tool reasoning if the provider supplied it, + // otherwise fall back to the shared response content. + let rationale = tool_call + .reasoning + .map(|r| { + let pre_truncated = truncate_at_tool_tags(&r); + clean_response(&pre_truncated) + }) + .filter(|r| !r.trim().is_empty()) + .unwrap_or_else(|| shared_reasoning.clone()); + ToolSelection { + tool_name: tool_call.name, + parameters: tool_call.arguments, + reasoning: rationale, + alternatives: vec![], + tool_call_id: tool_call.id, + } }) .collect(); @@ -664,13 +682,36 @@ Respond in JSON format: // If there were tool calls, return them for execution if !response.tool_calls.is_empty() { + let narrative = response.content.map(|c| { + let pre_truncated = truncate_at_tool_tags(&c); + clean_response(&pre_truncated) + }); + // Populate per-tool reasoning from the shared narrative when the + // provider did not supply per-tool rationale. + let tool_calls: Vec = response + .tool_calls + .into_iter() + .map(|mut tc| { + if tc.reasoning.as_ref().is_none_or(|r| r.trim().is_empty()) { + tc.reasoning = narrative.as_ref().filter(|n| !n.is_empty()).cloned(); + } else { + // Clean provider-supplied per-tool reasoning the same way + // we clean the shared narrative (strip thinking/tool tags). + tc.reasoning = tc + .reasoning + .map(|r| { + let pre_truncated = truncate_at_tool_tags(&r); + clean_response(&pre_truncated) + }) + .filter(|r| !r.trim().is_empty()); + } + tc + }) + .collect(); return Ok(RespondOutput { result: RespondResult::ToolCalls { - tool_calls: response.tool_calls, - content: response.content.map(|c| { - let pre_truncated = truncate_at_tool_tags(&c); - clean_response(&pre_truncated) - }), + tool_calls, + content: narrative, }, usage, }); @@ -1350,6 +1391,7 @@ fn recover_tool_calls_from_content( ), name: name.to_string(), arguments, + reasoning: None, }); continue; } @@ -1364,6 +1406,7 @@ fn recover_tool_calls_from_content( ), name: name.to_string(), arguments: serde_json::Value::Object(Default::default()), + reasoning: None, }); } } @@ -1401,6 +1444,7 @@ fn recover_tool_calls_from_content( ), name: name.to_string(), arguments, + reasoning: None, }); remaining = &args_start[bracket_end + 1..]; continue; @@ -1412,6 +1456,7 @@ fn recover_tool_calls_from_content( id: super::provider::generate_tool_call_id(calls.len(), RECOVERED_TOOL_CALL_SEED), name: name.to_string(), arguments: serde_json::Value::Object(Default::default()), + reasoning: None, }); remaining = after_name; } @@ -3145,4 +3190,32 @@ That's my plan."#; "Text {} middle " ); } + + /// Verify that reasoning normalization strips thinking tags and tool tags + /// from per-tool reasoning, matching the cleaning applied to shared reasoning. + #[test] + fn test_reasoning_normalization_strips_thinking_tags() { + let raw = "Let me consider...Search memory for prior context"; + let pre_truncated = truncate_at_tool_tags(raw); + let cleaned = clean_response(&pre_truncated); + assert!(!cleaned.contains("")); + assert!(cleaned.contains("Search memory")); + } + + #[test] + fn test_reasoning_normalization_strips_tool_tags() { + let raw = "Calling search {\"name\": \"search\"}"; + let pre_truncated = truncate_at_tool_tags(raw); + let cleaned = clean_response(&pre_truncated); + assert!(!cleaned.contains("")); + assert!(cleaned.contains("Calling search")); + } + + #[test] + fn test_reasoning_normalization_empty_after_cleaning() { + let raw = "internal only"; + let pre_truncated = truncate_at_tool_tags(raw); + let cleaned = clean_response(&pre_truncated); + assert!(cleaned.trim().is_empty()); + } } diff --git a/src/llm/rig_adapter.rs b/src/llm/rig_adapter.rs index a9030929..7a6b2ae8 100644 --- a/src/llm/rig_adapter.rs +++ b/src/llm/rig_adapter.rs @@ -490,6 +490,7 @@ fn extract_response( id: tc.id.clone(), name: tc.function.name.clone(), arguments: tc.function.arguments.clone(), + reasoning: None, }); } // Reasoning and Image variants are not mapped to IronClaw types @@ -880,6 +881,7 @@ mod tests { id: "Xt7mK9pQ2".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }; let msg = ChatMessage::assistant_with_tool_calls(Some("thinking".to_string()), vec![tc]); let messages = vec![msg]; @@ -997,6 +999,7 @@ mod tests { id: "".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }; let messages = vec![ChatMessage::assistant_with_tool_calls(None, vec![tc])]; let (_preamble, history) = convert_messages(&messages); @@ -1028,6 +1031,7 @@ mod tests { id: " ".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }; let messages = vec![ChatMessage::assistant_with_tool_calls(None, vec![tc])]; let (_preamble, history) = convert_messages(&messages); @@ -1061,6 +1065,7 @@ mod tests { id: "".to_string(), name: "search".to_string(), arguments: serde_json::json!({"query": "test"}), + reasoning: None, }; let assistant_msg = ChatMessage::assistant_with_tool_calls(None, vec![tc]); let tool_result_msg = ChatMessage { @@ -1380,11 +1385,13 @@ mod tests { id: "call_a".to_string(), name: "search".to_string(), arguments: serde_json::json!({"q": "rust"}), + reasoning: None, }; let tc2 = IronToolCall { id: "call_b".to_string(), name: "fetch".to_string(), arguments: serde_json::json!({"url": "https://example.com"}), + reasoning: None, }; let assistant = ChatMessage::assistant_with_tool_calls(None, vec![tc1, tc2]); let result_a = ChatMessage::tool_result("call_a", "search", "search results"); diff --git a/src/main.rs b/src/main.rs index eab01264..e885cb7d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -611,6 +611,7 @@ async fn async_main() -> anyhow::Result<()> { } else { GatewayChannel::new(gw_config.clone()) }; + gw = gw.with_owner_scope(config.owner_id.clone()); gw = gw.with_llm_provider(Arc::clone(&components.llm)); if let Some(ref ws) = components.workspace { gw = gw.with_workspace(Arc::clone(ws)); diff --git a/src/orchestrator/api.rs b/src/orchestrator/api.rs index dd9aba89..8da7ae6f 100644 --- a/src/orchestrator/api.rs +++ b/src/orchestrator/api.rs @@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize}; use tokio::sync::{Mutex, broadcast}; use uuid::Uuid; +use crate::channels::web::types::ToolDecisionDto; use crate::db::Database; use crate::llm::{CompletionRequest, LlmProvider, ToolCompletionRequest}; use crate::orchestrator::auth::{TokenStore, worker_auth_middleware}; @@ -277,9 +278,9 @@ async fn job_event_handler( }); } - // Convert to SSE event and broadcast + // Convert to app event and broadcast let job_id_str = job_id.to_string(); - let sse_event = match payload.event_type.as_str() { + let app_event = match payload.event_type.as_str() { "message" => AppEvent::JobMessage { job_id: job_id_str, role: payload @@ -344,6 +345,20 @@ async fn job_event_handler( // gain context/memory tracking capabilities. fallback_deliverable: payload.data.get("fallback_deliverable").cloned(), }, + "reasoning" => { + let narrative = payload + .data + .get("narrative") + .and_then(|v| v.as_str()) + .unwrap_or("") + .to_string(); + let decisions = ToolDecisionDto::from_json_array(&payload.data["decisions"]); + AppEvent::JobReasoning { + job_id: job_id_str, + narrative, + decisions, + } + } _ => AppEvent::JobStatus { job_id: job_id_str, message: payload @@ -390,9 +405,9 @@ async fn job_event_handler( }; if user_id.is_empty() { - let _ = tx.send((job_id, String::new(), sse_event)); + let _ = tx.send((job_id, String::new(), app_event)); } else { - let _ = tx.send((job_id, user_id, sse_event)); + let _ = tx.send((job_id, user_id, app_event)); } } diff --git a/src/tools/builtin/routine.rs b/src/tools/builtin/routine.rs index f4313483..bbc24139 100644 --- a/src/tools/builtin/routine.rs +++ b/src/tools/builtin/routine.rs @@ -915,7 +915,7 @@ fn parse_routine_create_request( fn build_routine_trigger(trigger: &NormalizedTriggerRequest) -> Trigger { match trigger { NormalizedTriggerRequest::Cron { schedule, timezone } => Trigger::Cron { - schedule: schedule.clone(), + schedule: normalize_cron_expression(schedule), timezone: timezone.clone(), }, NormalizedTriggerRequest::Manual => Trigger::Manual, @@ -1836,6 +1836,20 @@ mod tests { assert_eq!(parsed.cooldown_secs, 30); } + #[test] + fn build_routine_trigger_normalizes_cron_schedule() { + let trigger = build_routine_trigger(&NormalizedTriggerRequest::Cron { + schedule: "0 0 9 * * MON-FRI".to_string(), + timezone: Some("UTC".to_string()), + }); + + assert!(matches!( + trigger, + Trigger::Cron { schedule, timezone } + if schedule == "0 0 9 * * MON-FRI *" && timezone.as_deref() == Some("UTC") + )); + } + #[test] fn parses_grouped_message_event_with_tools() { let params = serde_json::json!({ diff --git a/src/tools/wasm/loader.rs b/src/tools/wasm/loader.rs index b50fc717..2a7ed040 100644 --- a/src/tools/wasm/loader.rs +++ b/src/tools/wasm/loader.rs @@ -418,6 +418,7 @@ fn resolve_oauth_refresh_config(cap_file: &CapabilitiesFile) -> Option Option, + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + // SAFETY: Tests use lock_env() to serialize environment access. + unsafe { + if let Some(ref value) = self.previous { + std::env::set_var(&self.key, value); + } else { + std::env::remove_var(&self.key); + } + } + } + } + + fn set_env_var(key: &str, value: Option<&str>) -> EnvVarGuard { + let previous = std::env::var(key).ok(); + // SAFETY: Tests use lock_env() to serialize environment access. + unsafe { + match value { + Some(value) => std::env::set_var(key, value), + None => std::env::remove_var(key), + } + } + EnvVarGuard { + key: key.to_string(), + previous, + } + } + #[test] fn wit_version_compat_none_is_ok() { // Pre-versioning extensions (no wit_version declared) should always pass @@ -871,6 +917,8 @@ mod tests { config.client_secret, Some(TEST_OAUTH_CLIENT_SECRET.to_string()) ); + assert_eq!(config.exchange_proxy_url, None); + assert_eq!(config.gateway_token, None); assert_eq!(config.secret_name, "google_oauth_token"); assert_eq!(config.provider, Some("google".to_string())); } @@ -931,6 +979,10 @@ mod tests { AuthCapabilitySchema, CapabilitiesFile, OAuthConfigSchema, }; + let _guard = lock_env(); + let _proxy_guard = set_env_var("IRONCLAW_OAUTH_EXCHANGE_URL", None); + let _gateway_token_guard = set_env_var("GATEWAY_AUTH_TOKEN", None); + // google_oauth_token should fall back to built-in credentials let caps = CapabilitiesFile { auth: Some(AuthCapabilitySchema { @@ -952,6 +1004,95 @@ mod tests { let config = config.unwrap(); assert!(!config.client_id.is_empty()); assert!(config.client_secret.is_some()); + assert_eq!(config.exchange_proxy_url, None); + assert_eq!(config.gateway_token, None); + } + + #[test] + fn test_resolve_oauth_refresh_config_hosted_proxy_populates_env_and_suppresses_builtin_secret() + { + use crate::tools::wasm::capabilities_schema::{ + AuthCapabilitySchema, CapabilitiesFile, OAuthConfigSchema, + }; + + let _guard = lock_env(); + let _proxy_guard = set_env_var( + "IRONCLAW_OAUTH_EXCHANGE_URL", + Some("https://compose-api.example.com"), + ); + let _gateway_token_guard = set_env_var("GATEWAY_AUTH_TOKEN", Some("gateway-test-token")); + let _client_id_guard = + set_env_var("GOOGLE_OAUTH_CLIENT_ID", Some("hosted-google-client-id")); + + let caps = CapabilitiesFile { + auth: Some(AuthCapabilitySchema { + secret_name: "google_oauth_token".to_string(), + provider: Some("google".to_string()), + oauth: Some(OAuthConfigSchema { + authorization_url: "https://accounts.google.com/o/oauth2/v2/auth".to_string(), + token_url: "https://oauth2.googleapis.com/token".to_string(), + client_id_env: Some("GOOGLE_OAUTH_CLIENT_ID".to_string()), + ..Default::default() + }), + ..Default::default() + }), + ..Default::default() + }; + + let config = super::resolve_oauth_refresh_config(&caps).expect("hosted oauth config"); + assert_eq!(config.client_id, "hosted-google-client-id"); + assert_eq!(config.client_secret, None); + assert_eq!( + config.exchange_proxy_url.as_deref(), + Some("https://compose-api.example.com") + ); + assert_eq!(config.gateway_token.as_deref(), Some("gateway-test-token")); + } + + #[test] + fn test_resolve_oauth_refresh_config_hosted_proxy_preserves_explicit_secret() { + use crate::tools::wasm::capabilities_schema::{ + AuthCapabilitySchema, CapabilitiesFile, OAuthConfigSchema, + }; + + let _guard = lock_env(); + let _proxy_guard = set_env_var( + "IRONCLAW_OAUTH_EXCHANGE_URL", + Some("https://compose-api.example.com"), + ); + let _gateway_token_guard = set_env_var("GATEWAY_AUTH_TOKEN", Some("gateway-test-token")); + let _client_id_guard = + set_env_var("GOOGLE_OAUTH_CLIENT_ID", Some("hosted-google-client-id")); + let _client_secret_guard = + set_env_var("GOOGLE_OAUTH_CLIENT_SECRET", Some("hosted-server-secret")); + + let caps = CapabilitiesFile { + auth: Some(AuthCapabilitySchema { + secret_name: "google_oauth_token".to_string(), + provider: Some("google".to_string()), + oauth: Some(OAuthConfigSchema { + authorization_url: "https://accounts.google.com/o/oauth2/v2/auth".to_string(), + token_url: "https://oauth2.googleapis.com/token".to_string(), + client_id_env: Some("GOOGLE_OAUTH_CLIENT_ID".to_string()), + client_secret_env: Some("GOOGLE_OAUTH_CLIENT_SECRET".to_string()), + ..Default::default() + }), + ..Default::default() + }), + ..Default::default() + }; + + let config = super::resolve_oauth_refresh_config(&caps).expect("hosted oauth config"); + assert_eq!(config.client_id, "hosted-google-client-id"); + assert_eq!( + config.client_secret.as_deref(), + Some("hosted-server-secret") + ); + assert_eq!( + config.exchange_proxy_url.as_deref(), + Some("https://compose-api.example.com") + ); + assert_eq!(config.gateway_token.as_deref(), Some("gateway-test-token")); } // --------------------------------------------------------------- diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index 33fcedb9..05508e97 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -19,7 +19,7 @@ use wasmtime_wasi::{ResourceTable, WasiCtx, WasiCtxBuilder, WasiView}; use crate::context::JobContext; use crate::llm::recording::{HttpExchangeRequest, HttpExchangeResponse, HttpInterceptor}; use crate::safety::LeakDetector; -use crate::secrets::SecretsStore; +use crate::secrets::{DecryptedSecret, SecretsStore}; use crate::tools::tool::{Tool, ToolError, ToolOutput}; use crate::tools::wasm::capabilities::Capabilities; use crate::tools::wasm::credential_injector::{ @@ -44,6 +44,7 @@ wasmtime::component::bindgen!({ }); // Alias the export interface types for convenience. +use crate::cli::oauth_defaults; use exports::near::agent::tool as wit_tool; /// Configuration needed to refresh an expired OAuth access token. @@ -59,6 +60,10 @@ pub struct OAuthRefreshConfig { pub client_id: String, /// OAuth client_secret (optional, some providers use PKCE without a secret). pub client_secret: Option, + /// Hosted OAuth proxy base URL (e.g., "http://host.docker.internal:8080"). + pub exchange_proxy_url: Option, + /// Gateway auth token for authenticating with the hosted OAuth proxy. + pub gateway_token: Option, /// Secret name of the access token (e.g., "google_oauth_token"). /// The refresh token lives at `{secret_name}_refresh_token`. pub secret_name: String, @@ -1210,6 +1215,53 @@ async fn refresh_oauth_token( user_id: &str, config: &OAuthRefreshConfig, ) -> bool { + let refresh_name = format!("{}_refresh_token", config.secret_name); + + if let Some(proxy_url) = config.exchange_proxy_url.as_deref() { + let Some(gateway_token) = config.gateway_token.as_deref() else { + tracing::warn!( + "OAuth refresh proxy is configured, but no gateway auth token is available" + ); + return false; + }; + + // In hosted mode, the configured exchange proxy owns the outbound token + // refresh and validation policy for the provider token_url. Direct-mode + // HTTPS/private-IP checks remain in place for self-hosted refreshes below. + let refresh_secret = match load_oauth_refresh_secret(store, user_id, &refresh_name).await { + Some(secret) => secret, + None => return false, + }; + let token_response = match oauth_defaults::refresh_token_via_proxy( + oauth_defaults::ProxyRefreshTokenRequest { + proxy_url, + gateway_token, + token_url: &config.token_url, + client_id: &config.client_id, + client_secret: config.client_secret.as_deref(), + refresh_token: refresh_secret.expose(), + provider: config.provider.as_deref(), + }, + ) + .await + { + Ok(response) => response, + Err(error) => { + tracing::warn!(error = %error, "OAuth token refresh via proxy failed"); + return false; + } + }; + + return persist_refreshed_oauth_tokens( + store, + user_id, + config, + &refresh_name, + token_response, + ) + .await; + } + // SSRF defense: token_url comes from the tool's capabilities file. if !config.token_url.starts_with("https://") { tracing::warn!( @@ -1227,19 +1279,6 @@ async fn refresh_oauth_token( return false; } - let refresh_name = format!("{}_refresh_token", config.secret_name); - let refresh_secret = match store.get_decrypted(user_id, &refresh_name).await { - Ok(s) => s, - Err(e) => { - tracing::debug!( - secret_name = %refresh_name, - error = %e, - "No refresh token available, skipping token refresh" - ); - return false; - } - }; - let client = match reqwest::Client::builder() .timeout(Duration::from_secs(15)) .redirect(reqwest::redirect::Policy::none()) @@ -1252,6 +1291,10 @@ async fn refresh_oauth_token( } }; + let refresh_secret = match load_oauth_refresh_secret(store, user_id, &refresh_name).await { + Some(secret) => secret, + None => return false, + }; let mut params = vec![ ("grant_type", "refresh_token".to_string()), ("refresh_token", refresh_secret.expose().to_string()), @@ -1287,22 +1330,55 @@ async fn refresh_oauth_token( return false; } }; - - let new_access_token = match token_data.get("access_token").and_then(|v| v.as_str()) { - Some(t) => t, + let token_response = match token_data.get("access_token").and_then(|v| v.as_str()) { + Some(access_token) => oauth_defaults::OAuthTokenResponse { + access_token: access_token.to_string(), + refresh_token: token_data + .get("refresh_token") + .and_then(|v| v.as_str()) + .map(str::to_string), + expires_in: token_data.get("expires_in").and_then(|v| v.as_u64()), + }, None => { tracing::warn!("Token refresh response missing access_token field"); return false; } }; - // Store the new access token with expiry + persist_refreshed_oauth_tokens(store, user_id, config, &refresh_name, token_response).await +} + +async fn load_oauth_refresh_secret( + store: &(dyn SecretsStore + Send + Sync), + user_id: &str, + refresh_name: &str, +) -> Option { + match store.get_decrypted(user_id, refresh_name).await { + Ok(secret) => Some(secret), + Err(error) => { + tracing::debug!( + secret_name = %refresh_name, + error = %error, + "No refresh token available, skipping token refresh" + ); + None + } + } +} + +async fn persist_refreshed_oauth_tokens( + store: &(dyn SecretsStore + Send + Sync), + user_id: &str, + config: &OAuthRefreshConfig, + refresh_name: &str, + token_response: oauth_defaults::OAuthTokenResponse, +) -> bool { let mut access_params = - crate::secrets::CreateSecretParams::new(&config.secret_name, new_access_token); + crate::secrets::CreateSecretParams::new(&config.secret_name, &token_response.access_token); if let Some(ref provider) = config.provider { access_params = access_params.with_provider(provider); } - if let Some(expires_in) = token_data.get("expires_in").and_then(|v| v.as_u64()) { + if let Some(expires_in) = token_response.expires_in { let expires_at = chrono::Utc::now() + chrono::Duration::seconds(expires_in as i64); access_params = access_params.with_expiry(expires_at); } @@ -1312,10 +1388,8 @@ async fn refresh_oauth_token( return false; } - // Store rotated refresh token if the provider sent a new one - if let Some(new_refresh) = token_data.get("refresh_token").and_then(|v| v.as_str()) { - let mut refresh_params = - crate::secrets::CreateSecretParams::new(&refresh_name, new_refresh); + if let Some(new_refresh) = token_response.refresh_token.as_deref() { + let mut refresh_params = crate::secrets::CreateSecretParams::new(refresh_name, new_refresh); if let Some(ref provider) = config.provider { refresh_params = refresh_params.with_provider(provider); } @@ -1664,9 +1738,18 @@ fn build_tool_usage_hint(tool_name: &str, schema: &serde_json::Value) -> String #[cfg(test)] mod tests { + use std::collections::HashMap; + use std::net::SocketAddr; use std::sync::{Arc, Mutex}; use async_trait::async_trait; + use axum::extract::{Form, State}; + use axum::http::HeaderMap; + use axum::routing::post; + use axum::{Json, Router}; + use serde_json::json; + use tokio::net::TcpListener; + use tokio::sync::{Mutex as AsyncMutex, oneshot}; use uuid::Uuid; use crate::context::JobContext; @@ -1756,6 +1839,95 @@ mod tests { } } + #[derive(Clone, Debug, PartialEq, Eq)] + struct RecordedProxyRequest { + authorization: Option, + form: HashMap, + } + + struct MockProxyServer { + addr: SocketAddr, + requests: Arc>>, + shutdown_tx: Option>, + server_task: Option>, + } + + impl MockProxyServer { + async fn start() -> Self { + async fn refresh_handler( + State(requests): State>>>, + headers: HeaderMap, + Form(form): Form>, + ) -> Json { + requests.lock().await.push(RecordedProxyRequest { + authorization: headers + .get(axum::http::header::AUTHORIZATION) + .and_then(|value| value.to_str().ok()) + .map(str::to_string), + form, + }); + Json(json!({ + "access_token": "mock-refreshed-access-token", + "refresh_token": "mock-rotated-refresh-token", + "expires_in": 3600 + })) + } + + let requests = Arc::new(AsyncMutex::new(Vec::new())); + let app = Router::new() + .route("/oauth/refresh", post(refresh_handler)) + .with_state(Arc::clone(&requests)); + + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock proxy"); + let addr = listener.local_addr().expect("read mock proxy addr"); + let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>(); + let server_task = tokio::spawn(async move { + let _ = axum::serve(listener, app) + .with_graceful_shutdown(async { + let _ = shutdown_rx.await; + }) + .await; + }); + + Self { + addr, + requests, + shutdown_tx: Some(shutdown_tx), + server_task: Some(server_task), + } + } + + fn base_url(&self) -> String { + format!("http://{}", self.addr) + } + + async fn requests(&self) -> Vec { + self.requests.lock().await.clone() + } + + async fn shutdown(mut self) { + if let Some(tx) = self.shutdown_tx.take() { + let _ = tx.send(()); + } + if let Some(task) = self.server_task.take() { + let _ = task.await; + } + } + } + + impl Drop for MockProxyServer { + fn drop(&mut self) { + if let Some(tx) = self.shutdown_tx.take() { + let _ = tx.send(()); + } + if let Some(task) = self.server_task.take() { + task.abort(); + } + } + } + #[test] fn test_wrapper_creation() { // This test verifies the runtime can be created @@ -2094,8 +2266,6 @@ mod tests { #[tokio::test] async fn test_resolve_host_credentials_bearer() { - use std::collections::HashMap; - use crate::secrets::{ CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, }; @@ -2141,8 +2311,6 @@ mod tests { #[tokio::test] async fn test_resolve_host_credentials_owner_scope_bearer() { - use std::collections::HashMap; - use crate::secrets::{ CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, }; @@ -2188,8 +2356,6 @@ mod tests { #[tokio::test] async fn test_execute_resolves_host_credentials_from_owner_scope_context() { - use std::collections::HashMap; - use crate::secrets::{CredentialLocation, CredentialMapping}; use crate::tools::wasm::capabilities::HttpCapability; @@ -2239,8 +2405,6 @@ mod tests { #[tokio::test] async fn test_resolve_host_credentials_missing_secret() { - use std::collections::HashMap; - use crate::secrets::{CredentialLocation, CredentialMapping}; use crate::tools::wasm::capabilities::HttpCapability; use crate::tools::wasm::wrapper::resolve_host_credentials; @@ -2272,8 +2436,6 @@ mod tests { #[tokio::test] async fn test_resolve_host_credentials_skips_refresh_when_not_expired() { - use std::collections::HashMap; - use crate::secrets::{ CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, }; @@ -2315,6 +2477,8 @@ mod tests { token_url: "https://oauth2.googleapis.com/token".to_string(), client_id: TEST_OAUTH_CLIENT_ID.to_string(), client_secret: Some(TEST_OAUTH_CLIENT_SECRET.to_string()), + exchange_proxy_url: None, + gateway_token: None, secret_name: "google_oauth_token".to_string(), provider: Some("google".to_string()), }; @@ -2331,8 +2495,6 @@ mod tests { #[tokio::test] async fn test_resolve_host_credentials_skips_refresh_no_config() { - use std::collections::HashMap; - use crate::secrets::{ CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, }; @@ -2376,8 +2538,6 @@ mod tests { #[tokio::test] async fn test_resolve_host_credentials_skips_refresh_no_expires_at() { - use std::collections::HashMap; - use crate::secrets::{ CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, }; @@ -2417,6 +2577,8 @@ mod tests { token_url: "https://oauth2.googleapis.com/token".to_string(), client_id: TEST_OAUTH_CLIENT_ID.to_string(), client_secret: Some(TEST_OAUTH_CLIENT_SECRET.to_string()), + exchange_proxy_url: None, + gateway_token: None, secret_name: "google_oauth_token".to_string(), provider: Some("google".to_string()), }; @@ -2431,6 +2593,249 @@ mod tests { ); } + #[tokio::test] + async fn test_resolve_host_credentials_refreshes_via_proxy_without_direct_token_url_validation() + { + use crate::secrets::{ + CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, + }; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::{OAuthRefreshConfig, resolve_host_credentials}; + + let proxy = MockProxyServer::start().await; + let store = test_secrets_store(); + + store + .create( + "user1", + CreateSecretParams::new("google_oauth_token", "expired-access-token") + .with_expiry(chrono::Utc::now() - chrono::Duration::hours(1)), + ) + .await + .unwrap(); + store + .create( + "user1", + CreateSecretParams::new("google_oauth_token_refresh_token", "stored-refresh-token"), + ) + .await + .unwrap(); + + let mut credentials = HashMap::new(); + credentials.insert( + "google_oauth_token".to_string(), + CredentialMapping { + secret_name: "google_oauth_token".to_string(), + location: CredentialLocation::AuthorizationBearer, + host_patterns: vec!["www.googleapis.com".to_string()], + }, + ); + + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + let oauth_config = OAuthRefreshConfig { + token_url: "http://127.0.0.1:9/provider-token-endpoint".to_string(), + client_id: "hosted-google-client-id".to_string(), + client_secret: None, + exchange_proxy_url: Some(proxy.base_url()), + gateway_token: Some("gateway-test-token".to_string()), + secret_name: "google_oauth_token".to_string(), + provider: Some("google".to_string()), + }; + + let resolved = + resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; + assert_eq!(resolved.len(), 1); + assert_eq!( + resolved[0].headers.get("Authorization"), + Some(&"Bearer mock-refreshed-access-token".to_string()) + ); + + let access_secret = store.get("user1", "google_oauth_token").await.unwrap(); + assert!( + access_secret + .expires_at + .expect("refreshed access token expiry") + > chrono::Utc::now() + ); + let access_value = store + .get_decrypted("user1", "google_oauth_token") + .await + .unwrap(); + assert_eq!(access_value.expose(), "mock-refreshed-access-token"); + + let refresh_value = store + .get_decrypted("user1", "google_oauth_token_refresh_token") + .await + .unwrap(); + assert_eq!(refresh_value.expose(), "mock-rotated-refresh-token"); + + let requests = proxy.requests().await; + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].authorization.as_deref(), + Some("Bearer gateway-test-token") + ); + assert_eq!( + requests[0].form.get("client_id").map(String::as_str), + Some("hosted-google-client-id") + ); + assert_eq!( + requests[0].form.get("token_url").map(String::as_str), + Some("http://127.0.0.1:9/provider-token-endpoint") + ); + assert_eq!( + requests[0].form.get("refresh_token").map(String::as_str), + Some("stored-refresh-token") + ); + assert_eq!( + requests[0].form.get("provider").map(String::as_str), + Some("google") + ); + assert!(!requests[0].form.contains_key("client_secret")); + + proxy.shutdown().await; + } + + #[tokio::test] + async fn test_resolve_host_credentials_skips_refresh_token_lookup_without_gateway_token() { + use crate::secrets::{ + CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, + }; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::{OAuthRefreshConfig, resolve_host_credentials}; + + let store = RecordingSecretsStore::new(); + + store + .create( + "user1", + CreateSecretParams::new("google_oauth_token", "expired-access-token") + .with_expiry(chrono::Utc::now() - chrono::Duration::hours(1)), + ) + .await + .unwrap(); + store + .create( + "user1", + CreateSecretParams::new("google_oauth_token_refresh_token", "stored-refresh-token"), + ) + .await + .unwrap(); + + let mut credentials = HashMap::new(); + credentials.insert( + "google_oauth_token".to_string(), + CredentialMapping { + secret_name: "google_oauth_token".to_string(), + location: CredentialLocation::AuthorizationBearer, + host_patterns: vec!["www.googleapis.com".to_string()], + }, + ); + + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + let oauth_config = OAuthRefreshConfig { + token_url: "https://oauth2.googleapis.com/token".to_string(), + client_id: "hosted-google-client-id".to_string(), + client_secret: None, + exchange_proxy_url: Some("https://compose-api.example.com".to_string()), + gateway_token: None, + secret_name: "google_oauth_token".to_string(), + provider: Some("google".to_string()), + }; + + let resolved = + resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; + assert!(resolved.is_empty()); + + let lookups = store.decrypted_lookups(); + assert!(lookups.contains(&("user1".to_string(), "google_oauth_token".to_string()))); + assert!(!lookups.contains(&( + "user1".to_string(), + "google_oauth_token_refresh_token".to_string(), + ))); + } + + #[tokio::test] + async fn test_resolve_host_credentials_skips_refresh_token_lookup_for_invalid_direct_token_url() + { + use crate::secrets::{ + CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, + }; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::{OAuthRefreshConfig, resolve_host_credentials}; + + let store = RecordingSecretsStore::new(); + + store + .create( + "user1", + CreateSecretParams::new("google_oauth_token", "expired-access-token") + .with_expiry(chrono::Utc::now() - chrono::Duration::hours(1)), + ) + .await + .unwrap(); + store + .create( + "user1", + CreateSecretParams::new("google_oauth_token_refresh_token", "stored-refresh-token"), + ) + .await + .unwrap(); + + let mut credentials = HashMap::new(); + credentials.insert( + "google_oauth_token".to_string(), + CredentialMapping { + secret_name: "google_oauth_token".to_string(), + location: CredentialLocation::AuthorizationBearer, + host_patterns: vec!["www.googleapis.com".to_string()], + }, + ); + + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + let oauth_config = OAuthRefreshConfig { + token_url: "http://127.0.0.1:9/provider-token-endpoint".to_string(), + client_id: TEST_OAUTH_CLIENT_ID.to_string(), + client_secret: Some(TEST_OAUTH_CLIENT_SECRET.to_string()), + exchange_proxy_url: None, + gateway_token: None, + secret_name: "google_oauth_token".to_string(), + provider: Some("google".to_string()), + }; + + let resolved = + resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; + assert!(resolved.is_empty()); + + let lookups = store.decrypted_lookups(); + assert!(lookups.contains(&("user1".to_string(), "google_oauth_token".to_string()))); + assert!(!lookups.contains(&( + "user1".to_string(), + "google_oauth_token_refresh_token".to_string(), + ))); + } + #[test] fn test_is_private_ip_v4() { use std::net::IpAddr; diff --git a/src/tunnel/cloudflare.rs b/src/tunnel/cloudflare.rs index 2c0ceb2a..9cc51bd4 100644 --- a/src/tunnel/cloudflare.rs +++ b/src/tunnel/cloudflare.rs @@ -111,10 +111,23 @@ impl Tunnel for CloudflareTunnel { } } - // Drain stderr in the background to prevent SIGPIPE/buffer stalls. - tokio::spawn(async move { while let Ok(Some(_)) = reader.next_line().await {} }); + if let Ok(mut guard) = self.url.write() { + *guard = Some(public_url.clone()); + } - // Drain stdout silently. + // We took ownership of cloudflared's stderr pipe above to parse the URL. + // cloudflared continues writing logs for its entire lifetime. If we drop + // the reader, the pipe closes and cloudflared gets SIGPIPE on its next + // write. We can't just store the reader without reading — the OS pipe + // buffer fills up and cloudflared blocks. So we drain it in a background + // task. The task exits naturally when cloudflared is killed (EOF). + let drain_handle = tokio::spawn(async move { + while let Ok(Some(line)) = reader.next_line().await { + tracing::trace!("cloudflared: {line}"); + } + }); + + // Drain stdout silently to prevent SIGPIPE/buffer stalls. if let Some(stdout) = stdout { tokio::spawn(async move { let mut out_reader = tokio::io::BufReader::new(stdout).lines(); @@ -122,12 +135,11 @@ impl Tunnel for CloudflareTunnel { }); } - if let Ok(mut guard) = self.url.write() { - *guard = Some(public_url.clone()); - } - let mut guard = self.proc.lock().await; - *guard = Some(TunnelProcess { child }); + *guard = Some(TunnelProcess { + child, + _pipe_drain: Some(drain_handle), + }); Ok(public_url) } diff --git a/src/tunnel/custom.rs b/src/tunnel/custom.rs index 9a2be403..2fffa264 100644 --- a/src/tunnel/custom.rs +++ b/src/tunnel/custom.rs @@ -73,6 +73,7 @@ impl Tunnel for CustomTunnel { let stderr = child.stderr.take(); let mut public_url = format!("http://{local_host}:{local_port}"); + let mut drain_handle: Option> = None; if self.url_pattern.is_some() && let Some(stdout) = stdout @@ -103,17 +104,26 @@ impl Tunnel for CustomTunnel { Err(_) => {} } } - // Drain remaining stdout to prevent SIGPIPE/buffer stalls. - tokio::spawn(async move { while let Ok(Some(_)) = reader.next_line().await {} }); + // We took ownership of the process's stdout pipe above to parse the + // URL. The process may continue writing to stdout for its lifetime. + // If we drop the reader, the pipe closes and the process gets SIGPIPE. + // We can't just store the reader without reading — the OS pipe buffer + // fills up and the process blocks. So we drain it in a background task. + // The task exits naturally when the process is killed (EOF). + drain_handle = Some(tokio::spawn(async move { + while let Ok(Some(line)) = reader.next_line().await { + tracing::trace!("custom-tunnel: {line}"); + } + })); } else if let Some(stdout) = stdout { - // No url_pattern: still drain stdout to prevent pipe stalls. + // No url_pattern: still drain stdout to prevent SIGPIPE/buffer stalls. tokio::spawn(async move { let mut reader = tokio::io::BufReader::new(stdout).lines(); while let Ok(Some(_)) = reader.next_line().await {} }); } - // Drain stderr silently. + // Drain stderr to prevent SIGPIPE/buffer stalls. if let Some(stderr) = stderr { tokio::spawn(async move { let mut reader = tokio::io::BufReader::new(stderr).lines(); @@ -126,7 +136,10 @@ impl Tunnel for CustomTunnel { } let mut guard = self.proc.lock().await; - *guard = Some(TunnelProcess { child }); + *guard = Some(TunnelProcess { + child, + _pipe_drain: drain_handle, + }); Ok(public_url) } diff --git a/src/tunnel/mod.rs b/src/tunnel/mod.rs index fa028834..8719b6e1 100644 --- a/src/tunnel/mod.rs +++ b/src/tunnel/mod.rs @@ -66,6 +66,10 @@ pub trait Tunnel: Send + Sync { /// Wraps a spawned tunnel child process. pub(crate) struct TunnelProcess { pub child: tokio::process::Child, + /// Background task that drains the process's output pipe (stdout or stderr). + /// Must stay alive or the process dies (SIGPIPE from closed pipe) or hangs + /// (OS pipe buffer fills up, blocking the process's writes). + pub _pipe_drain: Option>, } pub(crate) type SharedProcess = Arc>>; @@ -182,6 +186,22 @@ pub fn create_tunnel(config: &TunnelProviderConfig) -> Result (&str, u16) { + if let Some(ref http) = channels.http { + return (http.host.as_str(), http.port); + } + if let Some(ref gw) = channels.gateway { + return (gw.host.as_str(), gw.port); + } + ("0.0.0.0", 8080) +} + /// Start a managed tunnel if configured and no static URL is already set. /// /// Returns the (potentially mutated) config with `tunnel.public_url` set, @@ -201,28 +221,17 @@ pub async fn start_managed_tunnel( return (config, None); }; - let gateway_port = config - .channels - .gateway - .as_ref() - .map(|g| g.port) - .unwrap_or(3000); - let gateway_host = config - .channels - .gateway - .as_ref() - .map(|g| g.host.as_str()) - .unwrap_or("127.0.0.1"); + let (tunnel_host, tunnel_port) = resolve_tunnel_target(&config.channels); match create_tunnel(provider_config) { Ok(Some(tunnel)) => { tracing::debug!( "Starting {} tunnel on {}:{}...", tunnel.name(), - gateway_host, - gateway_port + tunnel_host, + tunnel_port ); - match tunnel.start(gateway_host, gateway_port).await { + match tunnel.start(tunnel_host, tunnel_port).await { Ok(url) => { tracing::debug!("Tunnel started: {}", url); config.tunnel.public_url = Some(url); @@ -383,10 +392,111 @@ mod tests { { let mut guard = proc.lock().await; - *guard = Some(TunnelProcess { child }); + *guard = Some(TunnelProcess { + child, + _pipe_drain: None, + }); } kill_shared(&proc).await.unwrap(); assert!(proc.lock().await.is_none()); } + + // ── Port selection regression tests ────────────────────────────── + + fn base_channels() -> crate::config::ChannelsConfig { + crate::config::ChannelsConfig { + cli: crate::config::CliConfig { enabled: false }, + http: None, + gateway: None, + signal: None, + wasm_channels_dir: std::env::temp_dir().join("ironclaw-test-channels"), + wasm_channels_enabled: false, + wasm_channel_owner_ids: std::collections::HashMap::new(), + } + } + + fn channels_with_http(host: &str, port: u16) -> crate::config::ChannelsConfig { + let mut c = base_channels(); + c.http = Some(crate::config::HttpConfig { + host: host.to_string(), + port, + webhook_secret: None, + user_id: "test".to_string(), + }); + c.gateway = Some(crate::config::GatewayConfig { + host: "127.0.0.1".to_string(), + port: 3000, + auth_token: None, + user_id: "test".to_string(), + workspace_read_scopes: Vec::new(), + memory_layers: Vec::new(), + user_tokens: None, + }); + c + } + + fn channels_gateway_only(host: &str, port: u16) -> crate::config::ChannelsConfig { + let mut c = base_channels(); + c.gateway = Some(crate::config::GatewayConfig { + host: host.to_string(), + port, + auth_token: None, + user_id: "test".to_string(), + workspace_read_scopes: Vec::new(), + memory_layers: Vec::new(), + user_tokens: None, + }); + c + } + + fn channels_neither() -> crate::config::ChannelsConfig { + base_channels() + } + + #[test] + fn tunnel_target_prefers_http_port() { + let channels = channels_with_http("0.0.0.0", 8080); + let (host, port) = resolve_tunnel_target(&channels); + assert_eq!(host, "0.0.0.0"); // safety: test-only + assert_eq!(port, 8080); // safety: test-only + } + + #[test] + fn tunnel_target_falls_back_to_gateway() { + let channels = channels_gateway_only("10.0.0.1", 4000); + let (host, port) = resolve_tunnel_target(&channels); + assert_eq!(host, "10.0.0.1"); // safety: test-only + assert_eq!(port, 4000); // safety: test-only + } + + #[test] + fn tunnel_target_defaults_to_webhook_fallback() { + let channels = channels_neither(); + let (host, port) = resolve_tunnel_target(&channels); + // Matches the webhook server's hardcoded fallback in main.rs + assert_eq!(host, "0.0.0.0"); // safety: test-only + assert_eq!(port, 8080); // safety: test-only + } + + #[test] + fn tunnel_target_http_takes_priority_over_gateway() { + let channels = channels_with_http("192.168.1.1", 9090); + let (host, port) = resolve_tunnel_target(&channels); + // Should use HTTP config, not gateway's 127.0.0.1:3000 + assert_eq!(host, "192.168.1.1"); // safety: test-only + assert_eq!(port, 9090); // safety: test-only + } + + #[test] + fn tunnel_target_no_http_no_gateway_matches_webhook_fallback() { + // When HTTP_PORT is not set and gateway is not configured (e.g. WASM + // channels exist but no explicit HTTP config), the webhook server in + // main.rs binds to 0.0.0.0:8080 as a hardcoded fallback. The tunnel + // must target the same address so webhook traffic reaches the right + // server. + let channels = channels_neither(); + let (host, port) = resolve_tunnel_target(&channels); + assert_eq!((host, port), ("0.0.0.0", 8080)); // safety: test-only + } } diff --git a/src/tunnel/ngrok.rs b/src/tunnel/ngrok.rs index 80a5cc46..66642e3b 100644 --- a/src/tunnel/ngrok.rs +++ b/src/tunnel/ngrok.rs @@ -110,12 +110,24 @@ impl Tunnel for NgrokTunnel { } } - // Drain stdout silently — ngrok only emits low-level connection events - // to stdout; the pipe must be consumed to prevent SIGPIPE/buffer stalls. - tokio::spawn(async move { while let Ok(Some(_)) = reader.next_line().await {} }); + if let Ok(mut guard) = self.url.write() { + *guard = Some(public_url.clone()); + } - // Drain stderr silently — with --log stdout all meaningful output goes - // to stdout; stderr only needs to be consumed to prevent pipe stalls. + // We took ownership of ngrok's stdout pipe above to parse the URL. + // ngrok continues writing logs to stdout for its entire lifetime. + // If we drop the reader, the pipe closes and ngrok gets SIGPIPE on + // its next write → process dies. We can't just store the reader + // without reading — the OS pipe buffer (~64KB) fills up and ngrok + // blocks. So we drain it in a background task. The task exits + // naturally when ngrok is killed (EOF on the pipe). + let drain_handle = tokio::spawn(async move { + while let Ok(Some(line)) = reader.next_line().await { + tracing::trace!("ngrok: {line}"); + } + }); + + // Drain stderr silently to prevent SIGPIPE/buffer stalls. if let Some(stderr) = stderr { tokio::spawn(async move { let mut err_reader = tokio::io::BufReader::new(stderr).lines(); @@ -123,12 +135,11 @@ impl Tunnel for NgrokTunnel { }); } - if let Ok(mut guard) = self.url.write() { - *guard = Some(public_url.clone()); - } - let mut guard = self.proc.lock().await; - *guard = Some(TunnelProcess { child }); + *guard = Some(TunnelProcess { + child, + _pipe_drain: Some(drain_handle), + }); Ok(public_url) } diff --git a/src/util.rs b/src/util.rs index 866f623c..a76f3b27 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,5 +1,7 @@ //! Shared utility functions used across the codebase. +use crate::llm::{ChatMessage, Role}; + /// Find the largest valid UTF-8 char boundary at or before `pos`. /// /// Polyfill for `str::floor_char_boundary` (nightly-only). Use when @@ -16,6 +18,17 @@ pub fn floor_char_boundary(s: &str, pos: usize) -> usize { i } +/// Ensure the last message in `messages` is a user-role message. +/// +/// NEAR AI rejects conversations that don't end with a user message; +/// Claude 4.6 rejects assistant prefill. Call this before any LLM +/// completion request to satisfy both requirements. +pub fn ensure_ends_with_user_message(messages: &mut Vec) { + if !matches!(messages.last(), Some(m) if m.role == Role::User) { + messages.push(ChatMessage::user("Continue.")); + } +} + /// Check if an LLM response explicitly signals that a job/task is complete. /// /// Uses phrase-level matching to avoid false positives from bare words like @@ -72,7 +85,8 @@ pub fn llm_signals_completion(response: &str) -> bool { #[cfg(test)] mod tests { - use crate::util::{floor_char_boundary, llm_signals_completion}; + use crate::llm::ChatMessage; + use crate::util::{ensure_ends_with_user_message, floor_char_boundary, llm_signals_completion}; // ── floor_char_boundary ── @@ -103,6 +117,42 @@ mod tests { assert_eq!(floor_char_boundary("", 5), 0); } + // ── ensure_ends_with_user_message ── + + #[test] + fn ensure_user_message_injects_when_empty() { + let mut msgs: Vec = vec![]; + ensure_ends_with_user_message(&mut msgs); + assert_eq!(msgs.len(), 1); + assert_eq!(msgs[0].role, crate::llm::Role::User); + } + + #[test] + fn ensure_user_message_injects_after_assistant() { + let mut msgs = vec![ChatMessage::user("hi"), ChatMessage::assistant("hello")]; + ensure_ends_with_user_message(&mut msgs); + assert_eq!(msgs.len(), 3); + assert_eq!(msgs[2].role, crate::llm::Role::User); + } + + #[test] + fn ensure_user_message_injects_after_tool_result() { + let mut msgs = vec![ + ChatMessage::user("run tool"), + ChatMessage::tool_result("call_1", "my_tool", "result"), + ]; + ensure_ends_with_user_message(&mut msgs); + assert_eq!(msgs.len(), 3); + assert_eq!(msgs[2].role, crate::llm::Role::User); + } + + #[test] + fn ensure_user_message_no_op_when_already_user() { + let mut msgs = vec![ChatMessage::user("hello")]; + ensure_ends_with_user_message(&mut msgs); + assert_eq!(msgs.len(), 1); + } + // ── llm_signals_completion ── #[test] diff --git a/src/worker/container.rs b/src/worker/container.rs index e0933975..5d8e03b5 100644 --- a/src/worker/container.rs +++ b/src/worker/container.rs @@ -151,7 +151,7 @@ Job: {} Description: {} You have tools for shell commands, file operations, and code editing. -Work independently to complete this job. Report when done."#, +Work independently to complete this job. When finished, your final message MUST include the phrase "The job is complete" to signal termination."#, job.title, job.description ))); @@ -373,6 +373,10 @@ impl LoopDelegate for ContainerDelegate { // Poll for follow-up prompts from the user self.poll_and_inject_prompt(reason_ctx).await; + // Claude 4.6 rejects assistant prefill; NEAR AI rejects any non-user-ending + // conversation. Ensure the last message is user-role before calling the LLM. + crate::util::ensure_ends_with_user_message(&mut reason_ctx.messages); + // Refresh tools (in case WASM tools were built) reason_ctx.available_tools = self.tools.tool_definitions().await; diff --git a/src/worker/job.rs b/src/worker/job.rs index f1843300..669c69f0 100644 --- a/src/worker/job.rs +++ b/src/worker/job.rs @@ -18,6 +18,7 @@ use crate::agent::agentic_loop::{ }; use crate::agent::scheduler::WorkerMessage; use crate::agent::task::TaskOutput; +use crate::channels::web::types::ToolDecisionDto; use crate::context::{ContextManager, JobState}; use crate::db::Database; use crate::error::Error; @@ -48,7 +49,7 @@ pub struct WorkerDeps { pub hooks: Arc, pub timeout: Duration, pub use_planning: bool, - /// SSE broadcast sender for live job event streaming to the web gateway. + /// Broadcast sender for live job event streaming to the web gateway. pub sse_tx: Option>, /// Approval context for tool execution. When `None`, all non-`Never` tools are /// blocked (legacy behavior). When `Some`, the context determines which tools @@ -200,6 +201,19 @@ impl Worker { .map(|s| s.to_string()), fallback_deliverable: data.get("fallback_deliverable").cloned(), }), + "reasoning" => { + let narrative = data + .get("narrative") + .and_then(|v| v.as_str()) + .unwrap_or("") + .to_string(); + let decisions = ToolDecisionDto::from_json_array(&data["decisions"]); + Some(AppEvent::JobReasoning { + job_id: job_id_str, + narrative, + decisions, + }) + } _ => None, }; if let Some(event) = event { @@ -897,6 +911,11 @@ Report when the job is complete or if you encounter issues you cannot resolve."# id: selection.tool_call_id.clone(), name: selection.tool_name.clone(), arguments: selection.parameters.clone(), + reasoning: if action.reasoning.is_empty() { + None + } else { + Some(action.reasoning.clone()) + }, }], )); @@ -1232,6 +1251,11 @@ impl<'a> LoopDelegate for JobDelegate<'a> { ) -> Option { // Refresh tool definitions so newly built tools become visible reason_ctx.available_tools = self.worker.tools().tool_definitions().await; + + // Claude 4.6 rejects assistant prefill; NEAR AI rejects any non-user-ending + // conversation. Ensure the last message is user-role before calling the LLM. + crate::util::ensure_ends_with_user_message(&mut reason_ctx.messages); + None } @@ -1352,6 +1376,48 @@ impl<'a> LoopDelegate for JobDelegate<'a> { ); } + // Emit reasoning event if any tool calls carry reasoning. + // Sanitize narrative and per-tool rationale through SafetyLayer + // (parity with ChatDelegate in dispatcher.rs). + let sanitized_narrative = content + .as_deref() + .filter(|c| !c.trim().is_empty()) + .map(|c| { + self.worker + .deps + .safety + .sanitize_tool_output("job_narrative", c) + .content + }) + .filter(|c| !c.trim().is_empty()) + .unwrap_or_default(); + let decisions: Vec = tool_calls + .iter() + .filter_map(|tc| { + tc.reasoning.as_ref().map(|r| { + let sanitized = self + .worker + .deps + .safety + .sanitize_tool_output("tool_rationale", r) + .content; + serde_json::json!({ + "tool_name": tc.name, + "rationale": sanitized, + }) + }) + }) + .collect(); + if !decisions.is_empty() { + self.worker.log_event( + "reasoning", + serde_json::json!({ + "narrative": sanitized_narrative, + "decisions": decisions, + }), + ); + } + // Add assistant message with tool_calls (OpenAI protocol) reason_ctx .messages @@ -1366,7 +1432,7 @@ impl<'a> LoopDelegate for JobDelegate<'a> { .map(|tc| ToolSelection { tool_name: tc.name.clone(), parameters: tc.arguments.clone(), - reasoning: String::new(), + reasoning: tc.reasoning.clone().unwrap_or_default(), alternatives: vec![], tool_call_id: tc.id.clone(), }) @@ -1419,6 +1485,11 @@ fn selections_to_tool_calls(selections: &[ToolSelection]) -> Vec { id: s.tool_call_id.clone(), name: s.tool_name.clone(), arguments: s.parameters.clone(), + reasoning: if s.reasoning.is_empty() { + None + } else { + Some(s.reasoning.clone()) + }, }) .collect() } diff --git a/src/workspace/mod.rs b/src/workspace/mod.rs index 0242047f..51d7d2fc 100644 --- a/src/workspace/mod.rs +++ b/src/workspace/mod.rs @@ -149,6 +149,7 @@ fn reject_if_injected(path: &str, content: &str) -> Result<(), WorkspaceError> { /// /// Allows Workspace to work with either a PostgreSQL `Repository` (the original /// path) or any `Database` trait implementation (e.g. libSQL backend). +#[derive(Clone)] enum WorkspaceStorage { /// PostgreSQL-backed repository (uses connection pool directly). #[cfg(feature = "postgres")] @@ -576,6 +577,60 @@ impl Workspace { self } + /// Clone the workspace configuration for a different primary user scope. + /// + /// This preserves search config, embeddings, shared read scopes, memory + /// layers, and privacy classifier while switching the primary read/write + /// scope to `user_id`. + pub fn scoped_to_user(&self, user_id: impl Into) -> Self { + let user_id = user_id.into(); + + let mut memory_layers = self.memory_layers.clone(); + for layer in &mut memory_layers { + if layer.sensitivity == crate::workspace::layer::LayerSensitivity::Private + && layer.scope == self.user_id + { + layer.scope = user_id.clone(); + } + } + + let mut read_user_ids = vec![user_id.clone()]; + for scope in &self.read_user_ids { + if scope != &self.user_id && !read_user_ids.contains(scope) { + read_user_ids.push(scope.clone()); + } + } + for scope in crate::workspace::layer::MemoryLayer::read_scopes(&memory_layers) { + if !read_user_ids.contains(&scope) { + read_user_ids.push(scope); + } + } + + let preserve_flags = user_id == self.user_id; + Self { + user_id, + read_user_ids, + agent_id: self.agent_id, + storage: self.storage.clone(), + embeddings: self.embeddings.clone(), + bootstrap_pending: std::sync::atomic::AtomicBool::new(if preserve_flags { + self.bootstrap_pending + .load(std::sync::atomic::Ordering::Acquire) + } else { + false + }), + bootstrap_completed: std::sync::atomic::AtomicBool::new(if preserve_flags { + self.bootstrap_completed + .load(std::sync::atomic::Ordering::Acquire) + } else { + false + }), + search_defaults: self.search_defaults.clone(), + memory_layers, + privacy_classifier: self.privacy_classifier.clone(), + } + } + /// Get the user ID (primary scope for writes). pub fn user_id(&self) -> &str { &self.user_id diff --git a/src/workspace/repository.rs b/src/workspace/repository.rs index 78ddfec5..13f6816b 100644 --- a/src/workspace/repository.rs +++ b/src/workspace/repository.rs @@ -15,6 +15,7 @@ use crate::workspace::document::{MemoryChunk, MemoryDocument, WorkspaceEntry}; use crate::workspace::search::{RankedResult, SearchConfig, SearchResult, fuse_results}; /// Database repository for workspace operations. +#[derive(Clone)] pub struct Repository { pool: Pool, } diff --git a/tests/batch_last_run_status_tests.rs b/tests/batch_last_run_status_tests.rs new file mode 100644 index 00000000..4bd476ec --- /dev/null +++ b/tests/batch_last_run_status_tests.rs @@ -0,0 +1,191 @@ +//! Tests for batch_get_last_run_status (#1469 N+1 fix). +//! +//! Verifies: +//! 1. Empty input returns empty map +//! 2. Returns the most recent run status per routine +//! 3. Routines with no runs are omitted from result +//! 4. Multiple routines with different statuses are correctly returned + +#[cfg(feature = "libsql")] +mod tests { + use std::sync::Arc; + + use chrono::{Duration, Utc}; + use uuid::Uuid; + + use ironclaw::agent::routine::{ + Routine, RoutineAction, RoutineGuardrails, RoutineRun, RunStatus, Trigger, + }; + use ironclaw::db::Database; + + async fn create_test_db() -> (Arc, tempfile::TempDir) { + use ironclaw::db::libsql::LibSqlBackend; + + let temp_dir = tempfile::tempdir().expect("tempdir"); + let db_path = temp_dir.path().join("test.db"); + let backend = LibSqlBackend::new_local(&db_path) + .await + .expect("LibSqlBackend"); + backend.run_migrations().await.expect("migrations"); + let db: Arc = Arc::new(backend); + (db, temp_dir) + } + + fn make_routine(id: Uuid) -> Routine { + Routine { + id, + name: format!("test-routine-{}", id), + description: "Test routine".to_string(), + user_id: "default".to_string(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::FullJob { + title: "Test job".to_string(), + description: "Test description".to_string(), + max_iterations: 5, + }, + guardrails: RoutineGuardrails { + cooldown: std::time::Duration::from_secs(0), + max_concurrent: 1, + dedup_window: None, + }, + notify: Default::default(), + last_run_at: None, + next_fire_at: None, + run_count: 0, + consecutive_failures: 0, + state: serde_json::json!({}), + created_at: Utc::now(), + updated_at: Utc::now(), + } + } + + fn make_run( + routine_id: Uuid, + status: RunStatus, + started_at: chrono::DateTime, + ) -> RoutineRun { + RoutineRun { + id: Uuid::new_v4(), + routine_id, + trigger_type: "manual".to_string(), + trigger_detail: None, + started_at, + completed_at: if status == RunStatus::Running { + None + } else { + Some(Utc::now()) + }, + status, + result_summary: None, + tokens_used: None, + job_id: None, + created_at: Utc::now(), + } + } + + #[tokio::test] + async fn test_batch_get_last_run_status_empty_input() { + let (db, _tmp) = create_test_db().await; + let result = db + .batch_get_last_run_status(&[]) + .await + .expect("batch query"); + assert!(result.is_empty()); + } + + #[tokio::test] + async fn test_batch_get_last_run_status_returns_latest() { + let (db, _tmp) = create_test_db().await; + + let routine_id = Uuid::new_v4(); + db.create_routine(&make_routine(routine_id)) + .await + .expect("create routine"); + + // Create an older run with Ok status + let older_run = make_run(routine_id, RunStatus::Ok, Utc::now() - Duration::hours(2)); + db.create_routine_run(&older_run) + .await + .expect("create older run"); + db.complete_routine_run(older_run.id, RunStatus::Ok, None, None) + .await + .expect("complete older run"); + + // Create a newer run with Attention status + let newer_run = make_run( + routine_id, + RunStatus::Attention, + Utc::now() - Duration::hours(1), + ); + db.create_routine_run(&newer_run) + .await + .expect("create newer run"); + db.complete_routine_run(newer_run.id, RunStatus::Attention, None, None) + .await + .expect("complete newer run"); + + let result = db + .batch_get_last_run_status(&[routine_id]) + .await + .expect("batch query"); + assert_eq!(result.get(&routine_id), Some(&RunStatus::Attention)); + } + + #[tokio::test] + async fn test_batch_get_last_run_status_omits_routines_without_runs() { + let (db, _tmp) = create_test_db().await; + + let with_runs = Uuid::new_v4(); + let without_runs = Uuid::new_v4(); + db.create_routine(&make_routine(with_runs)) + .await + .expect("create routine"); + db.create_routine(&make_routine(without_runs)) + .await + .expect("create routine"); + + let run = make_run(with_runs, RunStatus::Ok, Utc::now()); + db.create_routine_run(&run).await.expect("create run"); + db.complete_routine_run(run.id, RunStatus::Ok, None, None) + .await + .expect("complete run"); + + let result = db + .batch_get_last_run_status(&[with_runs, without_runs]) + .await + .expect("batch query"); + assert_eq!(result.get(&with_runs), Some(&RunStatus::Ok)); + assert_eq!(result.get(&without_runs), None); + } + + #[tokio::test] + async fn test_batch_get_last_run_status_multiple_routines() { + let (db, _tmp) = create_test_db().await; + + let r1 = Uuid::new_v4(); + let r2 = Uuid::new_v4(); + db.create_routine(&make_routine(r1)) + .await + .expect("create r1"); + db.create_routine(&make_routine(r2)) + .await + .expect("create r2"); + + let run1 = make_run(r1, RunStatus::Running, Utc::now()); + db.create_routine_run(&run1).await.expect("create run1"); + + let run2 = make_run(r2, RunStatus::Failed, Utc::now()); + db.create_routine_run(&run2).await.expect("create run2"); + db.complete_routine_run(run2.id, RunStatus::Failed, None, None) + .await + .expect("complete run2"); + + let result = db + .batch_get_last_run_status(&[r1, r2]) + .await + .expect("batch query"); + assert_eq!(result.get(&r1), Some(&RunStatus::Running)); + assert_eq!(result.get(&r2), Some(&RunStatus::Failed)); + } +} diff --git a/tests/e2e/CLAUDE.md b/tests/e2e/CLAUDE.md index 0cf5e6dc..46b7b752 100644 --- a/tests/e2e/CLAUDE.md +++ b/tests/e2e/CLAUDE.md @@ -53,6 +53,7 @@ HEADED=1 pytest scenarios/ | `test_skills.py` | Skills tab UI visibility, ClawHub search (skipped if registry unreachable), install + remove lifecycle | | `test_sse_reconnect.py` | SSE reconnects after programmatic `eventSource.close()` + `connectSSE()`; history is reloaded after reconnect | | `test_tool_approval.py` | Approval card appears, buttons disable on approve/deny, parameters toggle via `page.evaluate("showApproval(...)")`; the waiting-approval regression uses a real HTTP tool call | +| `test_oauth_refresh.py` | Hosted Gmail OAuth regression: complete setup via `/oauth/callback`, expire the stored access token in libSQL, trigger a real `gmail` tool call through `/api/chat/send`, and verify refresh goes through the mock `/oauth/refresh` proxy without forwarding `client_secret` | ## `helpers.py` @@ -75,6 +76,7 @@ All fixtures are defined in `tests/e2e/conftest.py`. Running `pytest scenarios/` | `ironclaw_binary` | Checks `target/debug/ironclaw`; if absent, runs `cargo build --no-default-features --features libsql` (timeout 600s). | | `mock_llm_server` | Starts `mock_llm.py --port 0`, reads the assigned port from stdout, waits for `/v1/models` to return 200. Yields the base URL. | | `ironclaw_server` | Starts the ironclaw binary with a minimal env (see below), waits for `/api/health` (timeout 60s). Yields the base URL. On teardown sends **SIGINT** (not SIGTERM) so the tokio ctrl_c handler triggers a graceful shutdown and LLVM coverage data is flushed. | +| `hosted_oauth_refresh_server` | Starts a second ironclaw instance with a dedicated libSQL DB and `GOOGLE_OAUTH_CLIENT_ID=hosted-google-client-id`, while still pointing `IRONCLAW_OAUTH_EXCHANGE_URL` at `mock_llm.py`. Yields a dict with `base_url`, `db_path`, `gateway_user_id`, and `mock_llm_url` for the hosted refresh regression scenario. | | `browser` | Launches a single Chromium instance (headless by default; set `HEADED=1` for headed). Shared across all tests. | ### Function-scoped fixtures @@ -100,6 +102,8 @@ EMBEDDING_ENABLED=false, SKILLS_ENABLED=true ONBOARD_COMPLETED=true # prevents setup wizard ``` +The `hosted_oauth_refresh_server` fixture uses the same baseline, but with its own DB/home tempdirs and `GOOGLE_OAUTH_CLIENT_ID=hosted-google-client-id` so hosted OAuth flows exercise proxy credential injection instead of the baked-in desktop Google app. + The binary is also started with `--no-onboard`. Coverage env vars (`CARGO_LLVM_COV*`, `LLVM_*`, `CARGO_ENCODED_RUSTFLAGS`, `CARGO_INCREMENTAL`) are forwarded from the outer environment when present. ## Mock LLM (`mock_llm.py`) @@ -113,6 +117,11 @@ python mock_llm.py --port 0 It serves `POST /v1/chat/completions` (streaming + non-streaming) and `GET /v1/models`. Responses are pattern-matched from `CANNED_RESPONSES` against the last user message. Unmatched messages return `"I understand your request."`. The model name reported is always `"mock-model"`. +It also hosts OAuth test endpoints: +- `POST /oauth/exchange` for hosted auth-code exchange +- `POST /oauth/refresh` for hosted refresh-token exchange +- `GET /__mock/oauth/state` and `POST /__mock/oauth/reset` so HTTP E2E scenarios can assert exact proxy payloads and reset counters between setup and refresh assertions + To add a new canned response: ```python # In mock_llm.py diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 06c7da03..aa8ba1cb 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -112,6 +112,39 @@ def _reserve_loopback_sockets(count: int) -> list[socket.socket]: sock.close() raise +async def _stop_process( + proc: asyncio.subprocess.Process, *, sig: int | None = None, timeout: float +) -> None: + """Signal a subprocess and wait briefly without masking exit races.""" + if proc.returncode is not None: + return + + try: + if sig is None: + proc.kill() + else: + proc.send_signal(sig) + except ProcessLookupError: + try: + await asyncio.wait_for(proc.wait(), timeout=timeout) + except asyncio.TimeoutError: + pass + return + + try: + await asyncio.wait_for(proc.wait(), timeout=timeout) + except asyncio.TimeoutError: + pass + + +def _forward_coverage_env(env: dict[str, str]) -> None: + """Forward cargo-llvm-cov env vars into child processes when present.""" + cov_env_prefixes = ("CARGO_LLVM_COV", "LLVM_") + cov_env_extras = ("CARGO_ENCODED_RUSTFLAGS", "CARGO_INCREMENTAL") + for key, val in os.environ.items(): + if key.startswith(cov_env_prefixes) or key in cov_env_extras: + env[key] = val + @pytest.fixture(scope="session") def ironclaw_binary(): @@ -264,14 +297,7 @@ async def ironclaw_server( "IRONCLAW_OAUTH_CALLBACK_URL": "https://oauth.test.example/oauth/callback", "IRONCLAW_OAUTH_EXCHANGE_URL": mock_llm_server, } - # Forward LLVM coverage instrumentation env vars when present - # (allows cargo-llvm-cov to collect profraw data from E2E runs). - # Use prefix matching to stay resilient to cargo-llvm-cov changes. - COV_ENV_PREFIXES = ("CARGO_LLVM_COV", "LLVM_") - COV_ENV_EXTRAS = ("CARGO_ENCODED_RUSTFLAGS", "CARGO_INCREMENTAL") - for key, val in os.environ.items(): - if key.startswith(COV_ENV_PREFIXES) or key in COV_ENV_EXTRAS: - env[key] = val + _forward_coverage_env(env) proc = await asyncio.create_subprocess_exec( ironclaw_binary, "--no-onboard", stdin=asyncio.subprocess.DEVNULL, @@ -279,35 +305,145 @@ async def ironclaw_server( stderr=asyncio.subprocess.PIPE, env=env, ) + startup_kill_attempted = False base_url = f"http://127.0.0.1:{gateway_port}" try: await wait_for_ready(f"{base_url}/api/health", timeout=60) yield base_url except TimeoutError: # Dump stderr so CI logs show why the server failed to start + if proc.returncode is None: + startup_kill_attempted = True + await _stop_process(proc, timeout=2) returncode = proc.returncode stderr_bytes = b"" if proc.stderr: try: stderr_bytes = await asyncio.wait_for(proc.stderr.read(8192), timeout=2) - except (asyncio.TimeoutError, Exception): + except asyncio.TimeoutError: pass stderr_text = stderr_bytes.decode("utf-8", errors="replace") - proc.kill() pytest.fail( f"ironclaw server failed to start on port {gateway_port} " f"(returncode={returncode}).\nstderr:\n{stderr_text}" ) finally: if proc.returncode is None: - # Use SIGINT (not SIGTERM) so tokio's ctrl_c handler triggers a - # graceful shutdown. This lets the LLVM coverage runtime run its - # atexit handler and flush .profraw files for cargo-llvm-cov. - proc.send_signal(signal.SIGINT) - try: - await asyncio.wait_for(proc.wait(), timeout=10) - except asyncio.TimeoutError: - proc.kill() + if startup_kill_attempted: + await _stop_process(proc, timeout=2) + else: + # Use SIGINT (not SIGTERM) so tokio's ctrl_c handler triggers a + # graceful shutdown. This lets the LLVM coverage runtime run its + # atexit handler and flush .profraw files for cargo-llvm-cov. + await _stop_process(proc, sig=signal.SIGINT, timeout=10) + if proc.returncode is None: + await _stop_process(proc, timeout=2) + + +@pytest.fixture(scope="session") +async def hosted_oauth_refresh_server( + ironclaw_binary, + mock_llm_server, + wasm_tools_dir, +): + """Start a hosted-mode ironclaw instance for OAuth refresh regression tests.""" + reserved = _reserve_loopback_sockets(2) + db_tmpdir = tempfile.TemporaryDirectory(prefix="ironclaw-e2e-hosted-oauth-db-") + home_tmpdir = tempfile.TemporaryDirectory(prefix="ironclaw-e2e-hosted-oauth-home-") + + try: + gateway_port = reserved[0].getsockname()[1] + http_port = reserved[1].getsockname()[1] + for sock in reserved: + if sock.fileno() != -1: + sock.close() + + db_path = os.path.join(db_tmpdir.name, "hosted-oauth-refresh.db") + home_dir = home_tmpdir.name + env = { + "PATH": os.environ.get("PATH", "/usr/bin:/bin"), + "HOME": home_dir, + "IRONCLAW_BASE_DIR": os.path.join(home_dir, ".ironclaw"), + "RUST_LOG": "ironclaw=info", + "RUST_BACKTRACE": "1", + "IRONCLAW_OWNER_ID": OWNER_SCOPE_ID, + "GATEWAY_ENABLED": "true", + "GATEWAY_HOST": "127.0.0.1", + "GATEWAY_PORT": str(gateway_port), + "GATEWAY_AUTH_TOKEN": AUTH_TOKEN, + "GATEWAY_USER_ID": OWNER_SCOPE_ID, + "HTTP_HOST": "127.0.0.1", + "HTTP_PORT": str(http_port), + "HTTP_WEBHOOK_SECRET": HTTP_WEBHOOK_SECRET, + "CLI_ENABLED": "false", + "LLM_BACKEND": "openai_compatible", + "LLM_BASE_URL": mock_llm_server, + "LLM_MODEL": "mock-model", + "DATABASE_BACKEND": "libsql", + "LIBSQL_PATH": db_path, + "SECRETS_MASTER_KEY": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + "SANDBOX_ENABLED": "false", + "SKILLS_ENABLED": "true", + "ROUTINES_ENABLED": "true", + "HEARTBEAT_ENABLED": "false", + "EMBEDDING_ENABLED": "false", + "WASM_ENABLED": "true", + "WASM_TOOLS_DIR": wasm_tools_dir, + "WASM_CHANNELS_DIR": _WASM_CHANNELS_TMPDIR.name, + "ONBOARD_COMPLETED": "true", + "IRONCLAW_OAUTH_CALLBACK_URL": "https://oauth.test.example/oauth/callback", + "IRONCLAW_OAUTH_EXCHANGE_URL": mock_llm_server, + "GOOGLE_OAUTH_CLIENT_ID": "hosted-google-client-id", + } + _forward_coverage_env(env) + + proc = await asyncio.create_subprocess_exec( + ironclaw_binary, "--no-onboard", + stdin=asyncio.subprocess.DEVNULL, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=env, + ) + startup_kill_attempted = False + base_url = f"http://127.0.0.1:{gateway_port}" + try: + await wait_for_ready(f"{base_url}/api/health", timeout=60) + yield { + "base_url": base_url, + "db_path": db_path, + "gateway_user_id": OWNER_SCOPE_ID, + "mock_llm_url": mock_llm_server, + } + except TimeoutError: + if proc.returncode is None: + startup_kill_attempted = True + await _stop_process(proc, timeout=2) + returncode = proc.returncode + stderr_bytes = b"" + if proc.stderr: + try: + stderr_bytes = await asyncio.wait_for(proc.stderr.read(8192), timeout=2) + except asyncio.TimeoutError: + pass + stderr_text = stderr_bytes.decode("utf-8", errors="replace") + pytest.fail( + f"hosted oauth refresh server failed to start on port {gateway_port} " + f"(returncode={returncode}).\nstderr:\n{stderr_text}" + ) + finally: + if proc.returncode is None: + if startup_kill_attempted: + await _stop_process(proc, timeout=2) + else: + await _stop_process(proc, sig=signal.SIGINT, timeout=10) + if proc.returncode is None: + await _stop_process(proc, timeout=2) + finally: + for sock in reserved: + if sock.fileno() != -1: + sock.close() + db_tmpdir.cleanup() + home_tmpdir.cleanup() @pytest.fixture(scope="session") @@ -362,12 +498,7 @@ async def http_channel_server_without_secret( "IRONCLAW_OAUTH_CALLBACK_URL": "https://oauth.test.example/oauth/callback", "IRONCLAW_OAUTH_EXCHANGE_URL": mock_llm_server, } - # Forward LLVM coverage instrumentation env vars when present - COV_ENV_PREFIXES = ("CARGO_LLVM_COV", "LLVM_") - COV_ENV_EXTRAS = ("CARGO_ENCODED_RUSTFLAGS", "CARGO_INCREMENTAL") - for key, val in os.environ.items(): - if key.startswith(COV_ENV_PREFIXES) or key in COV_ENV_EXTRAS: - env[key] = val + _forward_coverage_env(env) proc = await asyncio.create_subprocess_exec( ironclaw_binary, "--no-onboard", stdin=asyncio.subprocess.DEVNULL, @@ -375,6 +506,7 @@ async def http_channel_server_without_secret( stderr=asyncio.subprocess.PIPE, env=env, ) + startup_kill_attempted = False gateway_url = f"http://127.0.0.1:{gateway_port}" http_base_url = f"http://127.0.0.1:{http_port}" try: @@ -383,15 +515,17 @@ async def http_channel_server_without_secret( yield http_base_url except TimeoutError: # Dump stderr so CI logs show why the server failed to start + if proc.returncode is None: + startup_kill_attempted = True + await _stop_process(proc, timeout=2) returncode = proc.returncode stderr_bytes = b"" if proc.stderr: try: stderr_bytes = await asyncio.wait_for(proc.stderr.read(8192), timeout=2) - except (asyncio.TimeoutError, Exception): + except asyncio.TimeoutError: pass stderr_text = stderr_bytes.decode("utf-8", errors="replace") - proc.kill() pytest.fail( f"ironclaw server without webhook secret failed to start on ports " f"gateway={gateway_port}, http={http_port} " @@ -399,14 +533,15 @@ async def http_channel_server_without_secret( ) finally: if proc.returncode is None: - # Use SIGINT (not SIGTERM) so tokio's ctrl_c handler triggers a - # graceful shutdown. This lets the LLVM coverage runtime run its - # atexit handler and flush .profraw files for cargo-llvm-cov. - proc.send_signal(signal.SIGINT) - try: - await asyncio.wait_for(proc.wait(), timeout=10) - except asyncio.TimeoutError: - proc.kill() + if startup_kill_attempted: + await _stop_process(proc, timeout=2) + else: + # Use SIGINT (not SIGTERM) so tokio's ctrl_c handler triggers a + # graceful shutdown. This lets the LLVM coverage runtime run its + # atexit handler and flush .profraw files for cargo-llvm-cov. + await _stop_process(proc, sig=signal.SIGINT, timeout=10) + if proc.returncode is None: + await _stop_process(proc, timeout=2) @pytest.fixture(scope="session") diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py index 359c22d5..1147662c 100644 --- a/tests/e2e/mock_llm.py +++ b/tests/e2e/mock_llm.py @@ -34,6 +34,15 @@ TOOL_CALL_PATTERNS = [ "body": {"label": m.group("label")}, }, ), + ( + re.compile(r"check gmail unread|gmail unread", re.IGNORECASE), + "gmail", + lambda _: { + "action": "list_messages", + "query": "is:unread", + "max_results": 1, + }, + ), (re.compile(r"what time|current time", re.IGNORECASE), "time", lambda _: {"operation": "now"}), ( re.compile( @@ -91,6 +100,15 @@ TOOL_CALL_PATTERNS = [ ] +def _new_oauth_state() -> dict: + return { + "exchange_count": 0, + "refresh_count": 0, + "last_exchange": None, + "last_refresh": None, + } + + def _last_user_content(messages: list[dict]) -> str: for msg in reversed(messages): if msg.get("role") == "user": @@ -272,6 +290,12 @@ async def oauth_exchange(request: web.Request) -> web.Response: specific token params such as RFC 8707 `resource` are forwarded here. """ data = await request.post() + oauth_state = request.app["oauth_state"] + oauth_state["exchange_count"] += 1 + oauth_state["last_exchange"] = { + "authorization": request.headers.get("Authorization"), + "form": dict(data), + } code = data.get("code", "") access_token_field = data.get("access_token_field", "access_token") @@ -290,6 +314,39 @@ async def oauth_exchange(request: web.Request) -> web.Response: }) +async def oauth_refresh(request: web.Request) -> web.Response: + """Mock OAuth token refresh proxy for hosted refresh E2E tests.""" + data = await request.post() + oauth_state = request.app["oauth_state"] + oauth_state["refresh_count"] += 1 + oauth_state["last_refresh"] = { + "authorization": request.headers.get("Authorization"), + "form": dict(data), + } + + if request.headers.get("Authorization") != "Bearer e2e-test-token": + return web.json_response({"error": "invalid_gateway_auth"}, status=401) + if data.get("client_id") != "hosted-google-client-id": + return web.json_response({"error": "invalid_client_id"}, status=400) + if "client_secret" in data: + return web.json_response({"error": "unexpected_client_secret"}, status=400) + + return web.json_response({ + "access_token": "mock-refreshed-access-token", + "refresh_token": "mock-rotated-refresh-token", + "expires_in": 3600, + }) + + +async def oauth_state_handler(request: web.Request) -> web.Response: + return web.json_response(request.app["oauth_state"]) + + +async def oauth_reset(request: web.Request) -> web.Response: + request.app["oauth_state"] = _new_oauth_state() + return web.json_response({"ok": True}) + + async def models(_request: web.Request) -> web.Response: return web.json_response({ "object": "list", @@ -424,12 +481,16 @@ def main(): parser.add_argument("--port", type=int, default=0) args = parser.parse_args() app = web.Application() + app["oauth_state"] = _new_oauth_state() # Register both /v1/ and non-/v1/ paths (rig-core omits the /v1/ prefix) app.router.add_post("/v1/chat/completions", chat_completions) app.router.add_post("/chat/completions", chat_completions) app.router.add_get("/v1/models", models) app.router.add_get("/models", models) app.router.add_post("/oauth/exchange", oauth_exchange) + app.router.add_post("/oauth/refresh", oauth_refresh) + app.router.add_get("/__mock/oauth/state", oauth_state_handler) + app.router.add_post("/__mock/oauth/reset", oauth_reset) # Mock MCP server endpoints app.router.add_post("/mcp", mcp_endpoint) app.router.add_post("/mcp-400", mcp_endpoint_400) diff --git a/tests/e2e/scenarios/test_oauth_refresh.py b/tests/e2e/scenarios/test_oauth_refresh.py new file mode 100644 index 00000000..50871f7f --- /dev/null +++ b/tests/e2e/scenarios/test_oauth_refresh.py @@ -0,0 +1,227 @@ +"""Hosted OAuth refresh HTTP regression test. + +Runs a real ironclaw binary in hosted mode, expires a stored Gmail access +token in the libSQL database, triggers a real gmail tool call through the +chat API, and verifies that refresh uses the hosted proxy endpoint. +""" + +import asyncio +import sqlite3 +from datetime import datetime, timezone +from urllib.parse import parse_qs, urlparse + +import httpx + +from helpers import api_get, api_post + + +def _extract_state(auth_url: str) -> str: + parsed = urlparse(auth_url) + state = parse_qs(parsed.query).get("state", [None])[0] + assert state, f"auth_url should include state: {auth_url}" + return state + + +def _parse_timestamp(value: str | None) -> datetime | None: + if value is None: + return None + return datetime.fromisoformat(value.replace("Z", "+00:00")) + + +def _expire_access_token(db_path: str, user_id: str, secret_name: str) -> None: + with sqlite3.connect(db_path) as conn: + cursor = conn.execute( + """ + UPDATE secrets + SET expires_at = strftime('%Y-%m-%dT%H:%M:%fZ', 'now', '-1 hour') + WHERE user_id = ?1 AND name = ?2 + """, + (user_id, secret_name), + ) + conn.commit() + assert cursor.rowcount == 1, f"Expected one secret row for {user_id}/{secret_name}" + + +def _find_secret_row( + db_path: str, + secret_name: str, +) -> tuple[str, str | None, str | None]: + with sqlite3.connect(db_path) as conn: + row = conn.execute( + """ + SELECT user_id, expires_at, updated_at + FROM secrets + WHERE name = ?1 + ORDER BY updated_at DESC + LIMIT 1 + """, + (secret_name,), + ).fetchone() + assert row is not None, f"Missing secret row for {secret_name}" + return row[0], row[1], row[2] + + +async def _get_extension(base_url: str, name: str) -> dict | None: + response = await api_get(base_url, "/api/extensions", timeout=15) + response.raise_for_status() + for extension in response.json().get("extensions", []): + if extension["name"] == name: + return extension + return None + + +async def _reset_mock_oauth_state(mock_base_url: str) -> None: + async with httpx.AsyncClient() as client: + response = await client.post(f"{mock_base_url}/__mock/oauth/reset", timeout=10) + response.raise_for_status() + + +async def _get_mock_oauth_state(mock_base_url: str) -> dict: + async with httpx.AsyncClient() as client: + response = await client.get(f"{mock_base_url}/__mock/oauth/state", timeout=10) + response.raise_for_status() + return response.json() + + +async def _approve_pending_request(base_url: str, thread_id: str, request_id: str) -> None: + response = await api_post( + base_url, + "/api/chat/approval", + json={"request_id": request_id, "action": "approve", "thread_id": thread_id}, + timeout=15, + ) + assert response.status_code == 202, ( + f"Approval submission failed: {response.status_code} {response.text[:400]}" + ) + + +async def _wait_for_gmail_tool_call(base_url: str, thread_id: str, timeout: float = 30.0) -> dict: + approved_request_ids = set() + for _ in range(int(timeout * 2)): + response = await api_get( + base_url, + f"/api/chat/history?thread_id={thread_id}", + timeout=15, + ) + response.raise_for_status() + history = response.json() + + pending = history.get("pending_approval") + if pending and pending["request_id"] not in approved_request_ids: + await _approve_pending_request(base_url, thread_id, pending["request_id"]) + approved_request_ids.add(pending["request_id"]) + + for turn in history.get("turns", []): + for tool_call in turn.get("tool_calls", []): + if tool_call.get("name") == "gmail": + return history + + await asyncio.sleep(0.5) + + raise AssertionError(f"Timed out waiting for gmail tool call in thread {thread_id}") + + +async def _wait_for_refresh_request(mock_base_url: str, timeout: float = 20.0) -> dict: + for _ in range(int(timeout * 2)): + state = await _get_mock_oauth_state(mock_base_url) + if state.get("refresh_count") == 1: + return state + await asyncio.sleep(0.5) + raise AssertionError("Timed out waiting for exactly one OAuth refresh request") + + +async def test_hosted_gmail_oauth_refresh_uses_proxy(hosted_oauth_refresh_server): + server = hosted_oauth_refresh_server["base_url"] + db_path = hosted_oauth_refresh_server["db_path"] + mock_base_url = hosted_oauth_refresh_server["mock_llm_url"] + + install_response = await api_post( + server, + "/api/extensions/install", + json={"name": "gmail"}, + timeout=180, + ) + assert install_response.status_code == 200, install_response.text + assert install_response.json().get("success") is True + + setup_response = await api_post( + server, + "/api/extensions/gmail/setup", + json={"secrets": {}}, + timeout=30, + ) + assert setup_response.status_code == 200, setup_response.text + setup_data = setup_response.json() + assert setup_data.get("success") is True, setup_data + auth_url = setup_data.get("auth_url") + assert auth_url, setup_data + auth_params = parse_qs(urlparse(auth_url).query) + assert auth_params.get("client_id") == ["hosted-google-client-id"] + + async with httpx.AsyncClient() as client: + callback_response = await client.get( + f"{server}/oauth/callback", + params={"code": "mock_auth_code", "state": _extract_state(auth_url)}, + timeout=30, + follow_redirects=True, + ) + + assert callback_response.status_code == 200, callback_response.text[:400] + callback_body = callback_response.text.lower() + assert "connected" in callback_body or "success" in callback_body + + gmail = await _get_extension(server, "gmail") + assert gmail is not None, "gmail should be installed" + assert gmail["authenticated"] is True, gmail + assert "gmail" in gmail.get("tools", []), gmail + + await _reset_mock_oauth_state(mock_base_url) + + stored_user_id, expires_before, updated_before = _find_secret_row( + db_path, "google_oauth_token" + ) + assert _parse_timestamp(expires_before) is not None + assert _parse_timestamp(updated_before) is not None + + await asyncio.sleep(0.1) + _expire_access_token(db_path, stored_user_id, "google_oauth_token") + + thread_response = await api_post(server, "/api/chat/thread/new", timeout=15) + assert thread_response.status_code == 200, thread_response.text + thread_id = thread_response.json()["id"] + + send_response = await api_post( + server, + "/api/chat/send", + json={"content": "check gmail unread", "thread_id": thread_id}, + timeout=30, + ) + assert send_response.status_code == 202, send_response.text + + history = await _wait_for_gmail_tool_call(server, thread_id) + assert any( + tool_call.get("name") == "gmail" + for turn in history.get("turns", []) + for tool_call in turn.get("tool_calls", []) + ), history + + oauth_state = await _wait_for_refresh_request(mock_base_url) + assert oauth_state["refresh_count"] == 1, oauth_state + last_refresh = oauth_state["last_refresh"] + assert last_refresh is not None, oauth_state + assert last_refresh["authorization"] == "Bearer e2e-test-token" + assert last_refresh["form"]["client_id"] == "hosted-google-client-id" + assert "client_secret" not in last_refresh["form"], last_refresh + + refreshed_user_id, expires_after, updated_after = _find_secret_row( + db_path, "google_oauth_token" + ) + assert refreshed_user_id == stored_user_id + expires_after_dt = _parse_timestamp(expires_after) + updated_after_dt = _parse_timestamp(updated_after) + updated_before_dt = _parse_timestamp(updated_before) + assert expires_after_dt is not None + assert updated_after_dt is not None + assert updated_before_dt is not None + assert expires_after_dt > datetime.now(timezone.utc) + assert updated_after_dt > updated_before_dt diff --git a/tests/e2e/scenarios/test_telegram_hot_activation.py b/tests/e2e/scenarios/test_telegram_hot_activation.py index 261b837e..fede2be5 100644 --- a/tests/e2e/scenarios/test_telegram_hot_activation.py +++ b/tests/e2e/scenarios/test_telegram_hot_activation.py @@ -253,6 +253,6 @@ async def test_telegram_hot_activation_transitions_installed_to_active(page): assert await card.locator(SEL["ext_pairing_label"]).count() == 0 assert captured_setup_payloads == [ - {"secrets": {"telegram_bot_token": "123456789:ABCdefGhI"}}, - {"secrets": {}}, + {"secrets": {"telegram_bot_token": "123456789:ABCdefGhI"}, "fields": {}}, + {"secrets": {}, "fields": {}}, ] diff --git a/tests/e2e_advanced_traces.rs b/tests/e2e_advanced_traces.rs index b3efc8d9..ce18ad3d 100644 --- a/tests/e2e_advanced_traces.rs +++ b/tests/e2e_advanced_traces.rs @@ -587,6 +587,7 @@ mod advanced { async fn mcp_extension_lifecycle() { use crate::support::mock_mcp_server::{MockToolResponse, start_mock_mcp_server}; use ironclaw::extensions::{AuthHint, ExtensionKind, ExtensionSource, RegistryEntry}; + const TEST_USER_ID: &str = "test-user"; // 1. Start mock MCP server with pre-configured tool responses. let mock_server = start_mock_mcp_server(vec![ @@ -654,14 +655,14 @@ mod advanced { ext_mgr .secrets() .create( - "default", + TEST_USER_ID, ironclaw::secrets::CreateSecretParams::new(secret_name, "mock-access-token") .with_provider("mcp:mock-notion".to_string()), ) .await .expect("failed to inject test token"); - let activate_result = ext_mgr.activate("mock-notion", "default").await; + let activate_result = ext_mgr.activate("mock-notion", TEST_USER_ID).await; assert!( activate_result.is_ok(), "activation failed: {:?}", diff --git a/tests/e2e_builtin_tool_coverage.rs b/tests/e2e_builtin_tool_coverage.rs index 42d7fb75..1c3cc6a2 100644 --- a/tests/e2e_builtin_tool_coverage.rs +++ b/tests/e2e_builtin_tool_coverage.rs @@ -439,7 +439,7 @@ mod tests { match &routine.trigger { Trigger::Cron { schedule, timezone } => { - assert_eq!(schedule, "0 0 9 * * MON-FRI"); + assert_eq!(schedule, "0 0 9 * * MON-FRI *"); assert_eq!(timezone.as_deref(), Some("UTC")); } other => panic!("expected cron trigger, got {other:?}"), diff --git a/tests/e2e_routine_heartbeat.rs b/tests/e2e_routine_heartbeat.rs index 12125d43..27d8cfdc 100644 --- a/tests/e2e_routine_heartbeat.rs +++ b/tests/e2e_routine_heartbeat.rs @@ -561,11 +561,7 @@ mod tests { "deploy to production now", ); let fired = engine - .check_event_triggers( - &matching_msg.user_id, - &matching_msg.channel, - &matching_msg.content, - ) + .check_event_triggers(&matching_msg, &matching_msg.content) .await; assert!( fired >= 1, @@ -584,11 +580,7 @@ mod tests { "check the staging environment", ); let fired_neg = engine - .check_event_triggers( - &non_matching_msg.user_id, - &non_matching_msg.channel, - &non_matching_msg.content, - ) + .check_event_triggers(&non_matching_msg, &non_matching_msg.content) .await; assert_eq!(fired_neg, 0, "Expected 0 routines fired on non-match"); } @@ -652,7 +644,7 @@ mod tests { "deploy to production now", ); let guest_fired = engine - .check_event_triggers(&guest_msg.user_id, &guest_msg.channel, &guest_msg.content) + .check_event_triggers(&guest_msg, &guest_msg.content) .await; assert_eq!( guest_fired, 0, @@ -677,7 +669,7 @@ mod tests { "deploy to production now", ); let owner_fired = engine - .check_event_triggers(&owner_msg.user_id, &owner_msg.channel, &owner_msg.content) + .check_event_triggers(&owner_msg, &owner_msg.content) .await; assert!( owner_fired >= 1, @@ -906,9 +898,7 @@ mod tests { "default", "test-cooldown trigger", ); - let fired1 = engine - .check_event_triggers(&msg.user_id, &msg.channel, &msg.content) - .await; + let fired1 = engine.check_event_triggers(&msg, &msg.content).await; assert!(fired1 >= 1, "First fire should work"); // Give spawn time, then update last_run_at to simulate recent execution. @@ -923,9 +913,7 @@ mod tests { engine.refresh_event_cache().await; // Second fire should be blocked by cooldown. - let fired2 = engine - .check_event_triggers(&msg.user_id, &msg.channel, &msg.content) - .await; + let fired2 = engine.check_event_triggers(&msg, &msg.content).await; assert_eq!(fired2, 0, "Second fire should be blocked by cooldown"); } @@ -1095,9 +1083,7 @@ mod tests { engine.refresh_event_cache().await; let msg = IncomingMessage::new("test", "default", "DISABLE_ME"); - let fired_before = engine - .check_event_triggers(&msg.user_id, &msg.channel, &msg.content) - .await; + let fired_before = engine.check_event_triggers(&msg, &msg.content).await; assert!(fired_before >= 1, "Expected routine to fire before disable"); // Simulate what routines_toggle_handler now does: update DB, then refresh. @@ -1106,9 +1092,7 @@ mod tests { db.update_routine(&routine).await.expect("update_routine"); engine.refresh_event_cache().await; - let fired_after = engine - .check_event_triggers(&msg.user_id, &msg.channel, &msg.content) - .await; + let fired_after = engine.check_event_triggers(&msg, &msg.content).await; assert_eq!( fired_after, 0, "Disabled routine must not fire after cache refresh" @@ -1134,10 +1118,7 @@ mod tests { let msg = IncomingMessage::new("test", "default", "DELETE_ME"); assert!( - engine - .check_event_triggers(&msg.user_id, &msg.channel, &msg.content) - .await - >= 1, + engine.check_event_triggers(&msg, &msg.content).await >= 1, "Expected routine to fire before delete" ); @@ -1146,9 +1127,7 @@ mod tests { engine.refresh_event_cache().await; assert_eq!( - engine - .check_event_triggers(&msg.user_id, &msg.channel, &msg.content) - .await, + engine.check_event_triggers(&msg, &msg.content).await, 0, "Deleted routine must not fire after cache refresh" ); @@ -1462,8 +1441,9 @@ mod tests { db.create_routine(&routine).await.expect("create_routine"); engine.refresh_event_cache().await; + let trigger_msg = IncomingMessage::new("test", "default", "owner-gate"); let fired = engine - .check_event_triggers("default", "test", "owner-gate") + .check_event_triggers(&trigger_msg, &trigger_msg.content) .await; assert_eq!(fired, 1, "expected one matching event routine"); diff --git a/tests/e2e_workspace_coverage.rs b/tests/e2e_workspace_coverage.rs index 396b676e..68956d30 100644 --- a/tests/e2e_workspace_coverage.rs +++ b/tests/e2e_workspace_coverage.rs @@ -12,6 +12,7 @@ mod tests { use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; + use ironclaw::workspace::Workspace; // ----------------------------------------------------------------------- // Test 1: write_chunk_search @@ -268,6 +269,7 @@ mod tests { #[tokio::test] async fn identity_in_system_prompt() { + const TEST_USER_ID: &str = "test-user"; let trace = LlmTrace::from_file(concat!( env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/llm_traces/workspace/identity_prompt.json" @@ -280,7 +282,7 @@ mod tests { .await; // Seed an IDENTITY.md so the system prompt has real content to inject. - let ws = rig.workspace().expect("workspace must be available"); + let ws = Workspace::new_with_db(TEST_USER_ID, rig.database().clone()); ws.write( "IDENTITY.md", "I am TestBot, a helpful testing assistant created for E2E verification.", diff --git a/tests/multi_tenant_integration.rs b/tests/multi_tenant_integration.rs index 76e28f3c..227fa721 100644 --- a/tests/multi_tenant_integration.rs +++ b/tests/multi_tenant_integration.rs @@ -19,10 +19,13 @@ use axum::middleware; use axum::routing::{get, post}; use tower::ServiceExt; +use ironclaw::channels::IncomingMessage; use ironclaw::channels::web::auth::{ AuthenticatedUser, MultiAuthState, UserIdentity, auth_middleware, }; -use ironclaw::channels::web::server::{GatewayState, PerUserRateLimiter, RateLimiter}; +use ironclaw::channels::web::server::{ + GatewayState, PerUserRateLimiter, RateLimiter, start_server, +}; use ironclaw::channels::web::sse::SseManager; use ironclaw::channels::web::test_helpers::TestGatewayBuilder; use ironclaw::channels::web::ws::WsConnectionTracker; @@ -37,6 +40,9 @@ const ALICE_TOKEN: &str = "tok-alice-secret"; const BOB_TOKEN: &str = "tok-bob-secret"; const ALICE_USER_ID: &str = "alice"; const BOB_USER_ID: &str = "bob"; +const OWNER_TOKEN: &str = "tok-owner-secret"; +const OWNER_SCOPE_ID: &str = "owner-scope"; +const GATEWAY_SENDER_ID: &str = "gateway-sender"; /// Build a MultiAuthState with two users. fn two_user_auth() -> MultiAuthState { @@ -537,7 +543,8 @@ fn gateway_state_has_multi_tenant_fields() { job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: "fallback".to_string(), // Multi-tenant: renamed from user_id + owner_id: "fallback".to_string(), + default_sender_id: "fallback".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: None, @@ -553,7 +560,8 @@ fn gateway_state_has_multi_tenant_fields() { active_config: Default::default(), }; - assert_eq!(state.default_user_id, "fallback"); + assert_eq!(state.owner_id, "fallback"); + assert_eq!(state.default_sender_id, "fallback"); assert!(state.workspace_pool.is_none()); } @@ -572,6 +580,69 @@ async fn start_multi_user_server() -> (SocketAddr, Arc) { .expect("Failed to start multi-user test server") } +async fn start_owner_scoped_sender_server() -> ( + SocketAddr, + Arc, + tokio::sync::mpsc::Receiver, +) { + let (agent_tx, agent_rx) = tokio::sync::mpsc::channel(64); + + let mut tokens = HashMap::new(); + tokens.insert( + OWNER_TOKEN.to_string(), + UserIdentity { + user_id: OWNER_SCOPE_ID.to_string(), + workspace_read_scopes: Vec::new(), + }, + ); + tokens.insert( + BOB_TOKEN.to_string(), + UserIdentity { + user_id: BOB_USER_ID.to_string(), + workspace_read_scopes: Vec::new(), + }, + ); + + let state = Arc::new(GatewayState { + msg_tx: tokio::sync::RwLock::new(Some(agent_tx)), + sse: Arc::new(SseManager::new()), + workspace: None, + workspace_pool: None, + session_manager: None, + log_broadcaster: None, + log_level_handle: None, + extension_manager: None, + tool_registry: None, + store: None, + job_manager: None, + prompt_queue: None, + scheduler: None, + owner_id: OWNER_SCOPE_ID.to_string(), + default_sender_id: GATEWAY_SENDER_ID.to_string(), + shutdown_tx: tokio::sync::RwLock::new(None), + ws_tracker: Some(Arc::new(WsConnectionTracker::new())), + llm_provider: None, + skill_registry: None, + skill_catalog: None, + chat_rate_limiter: PerUserRateLimiter::new(30, 60), + oauth_rate_limiter: RateLimiter::new(10, 60), + webhook_rate_limiter: RateLimiter::new(10, 60), + registry_entries: Vec::new(), + cost_guard: None, + routine_engine: Arc::new(tokio::sync::RwLock::new(None)), + startup_time: std::time::Instant::now(), + active_config: Default::default(), + }); + + let auth = MultiAuthState::multi(tokens); + let addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); + let bound = start_server(addr, state.clone(), auth) + .await + .expect("Failed to start owner-scoped sender test server"); + + (bound, state, agent_rx) +} + #[tokio::test] async fn full_server_alice_can_access_protected_endpoint() { let (addr, _state) = start_multi_user_server().await; @@ -677,6 +748,49 @@ async fn full_server_chat_send_accepted_for_alice() { assert_eq!(msg.channel, "gateway"); } +#[tokio::test] +async fn full_server_chat_send_rewrites_sender_only_for_owner_scope_rebind() { + let (addr, _state, mut agent_rx) = start_owner_scoped_sender_server().await; + + let client = reqwest::Client::new(); + + let owner_resp = client + .post(format!("http://{}/api/chat/send", addr)) + .header("Authorization", format!("Bearer {}", OWNER_TOKEN)) + .header("Content-Type", "application/json") + .body(r#"{"content":"hello from owner"}"#) + .send() + .await + .unwrap(); + assert_eq!(owner_resp.status(), 202); + + let owner_msg = tokio::time::timeout(Duration::from_secs(2), agent_rx.recv()) + .await + .expect("Timed out waiting for owner message") + .expect("Agent channel closed"); + assert_eq!(owner_msg.user_id, OWNER_SCOPE_ID); + assert_eq!(owner_msg.sender_id, GATEWAY_SENDER_ID); + assert_eq!(owner_msg.content, "hello from owner"); + + let other_resp = client + .post(format!("http://{}/api/chat/send", addr)) + .header("Authorization", format!("Bearer {}", BOB_TOKEN)) + .header("Content-Type", "application/json") + .body(r#"{"content":"hello from bob"}"#) + .send() + .await + .unwrap(); + assert_eq!(other_resp.status(), 202); + + let other_msg = tokio::time::timeout(Duration::from_secs(2), agent_rx.recv()) + .await + .expect("Timed out waiting for non-owner message") + .expect("Agent channel closed"); + assert_eq!(other_msg.user_id, BOB_USER_ID); + assert_eq!(other_msg.sender_id, BOB_USER_ID); + assert_eq!(other_msg.content, "hello from bob"); +} + #[tokio::test] async fn full_server_chat_send_rejected_without_auth() { let (addr, _state) = start_multi_user_server().await; @@ -888,7 +1002,8 @@ async fn start_multi_user_server_with_db() -> ( job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: ALICE_USER_ID.to_string(), + owner_id: ALICE_USER_ID.to_string(), + default_sender_id: ALICE_USER_ID.to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: None, diff --git a/tests/multi_tenant_system_prompt.rs b/tests/multi_tenant_system_prompt.rs index ece794bf..b89e6cb5 100644 --- a/tests/multi_tenant_system_prompt.rs +++ b/tests/multi_tenant_system_prompt.rs @@ -1,10 +1,10 @@ -//! Tests proving that multi-tenant system prompts are broken. +//! Regression tests for multi-tenant system prompts. //! -//! Bug: In multi-tenant mode, the agent loop uses `self.workspace()` which -//! returns a single shared workspace (user_id="default"). Identity files -//! (IDENTITY.md, SOUL.md, USER.md) seeded under per-user IDs ("alice", -//! "bob") are invisible to this workspace, so the system prompt is -//! empty/wrong. +//! The agent must build the conversational system prompt from a workspace +//! scoped to the incoming message's user, not from the shared owner-scope +//! workspace created at startup. Otherwise per-user identity files +//! (IDENTITY.md, SOUL.md, USER.md) become invisible and different users can +//! see the same owner-scoped prompt. //! //! These tests: //! 1. Seed identity files for two users (alice, bob) in the database @@ -13,7 +13,7 @@ //! correct user's identity //! 4. Verify user A's identity doesn't leak into user B's prompt //! -//! All tests are expected to FAIL until the bug is fixed. +//! These tests ensure each user's identity is isolated correctly. #[cfg(feature = "libsql")] mod support; diff --git a/tests/openai_compat_integration.rs b/tests/openai_compat_integration.rs index 16568246..b677e57f 100644 --- a/tests/openai_compat_integration.rs +++ b/tests/openai_compat_integration.rs @@ -94,6 +94,7 @@ impl LlmProvider for MockLlmProvider { id: "call_mock_001".to_string(), name: tool.name.clone(), arguments: serde_json::json!({"test": true}), + reasoning: None, }], input_tokens: 15, output_tokens: 8, @@ -203,7 +204,8 @@ async fn start_test_server_with_provider( job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: "test-user".to_string(), + owner_id: "test-user".to_string(), + default_sender_id: "test-user".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: Some(llm_provider), @@ -701,7 +703,8 @@ async fn test_no_llm_provider_returns_503() { job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: "test-user".to_string(), + owner_id: "test-user".to_string(), + default_sender_id: "test-user".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: None, // No LLM! diff --git a/tests/support/gateway_workflow_harness.rs b/tests/support/gateway_workflow_harness.rs index e4620f70..5f477de0 100644 --- a/tests/support/gateway_workflow_harness.rs +++ b/tests/support/gateway_workflow_harness.rs @@ -226,7 +226,8 @@ impl GatewayWorkflowHarness { job_manager: None, prompt_queue: None, scheduler: Some(scheduler_slot.clone()), - default_user_id: user_id.clone(), + owner_id: user_id.clone(), + default_sender_id: user_id.clone(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: Some(Arc::clone(&components.llm)), diff --git a/tests/support/trace_llm.rs b/tests/support/trace_llm.rs index e33caf6b..239cfdb5 100644 --- a/tests/support/trace_llm.rs +++ b/tests/support/trace_llm.rs @@ -566,6 +566,7 @@ impl LlmProvider for TraceLlm { id: tc.id, name: tc.name, arguments: tc.arguments, + reasoning: None, }) .collect(); Ok(ToolCompletionResponse { diff --git a/tests/ws_gateway_integration.rs b/tests/ws_gateway_integration.rs index 9c4b8e76..0ec5c929 100644 --- a/tests/ws_gateway_integration.rs +++ b/tests/ws_gateway_integration.rs @@ -5,7 +5,7 @@ //! - WebSocket upgrade with auth //! - Ping/pong //! - Client message → agent msg_tx -//! - Broadcast SSE event → WebSocket client +//! - Broadcast AppEvent → WebSocket client //! - Connection tracking (counter increment/decrement) //! - Gateway status endpoint @@ -51,7 +51,8 @@ async fn start_test_server() -> ( job_manager: None, prompt_queue: None, scheduler: None, - default_user_id: "test-user".to_string(), + owner_id: "test-user".to_string(), + default_sender_id: "test-user".to_string(), shutdown_tx: tokio::sync::RwLock::new(None), ws_tracker: Some(Arc::new(WsConnectionTracker::new())), llm_provider: None, @@ -163,7 +164,7 @@ async fn test_ws_broadcast_event_received() { // Give the connection a moment to fully establish tokio::time::sleep(Duration::from_millis(50)).await; - // Broadcast an SSE event (simulates agent sending a response) + // Broadcast an event (simulates agent sending a response) state.sse.broadcast(AppEvent::Response { content: "agent says hi".to_string(), thread_id: "t1".to_string(),