diff --git a/src/agent/routine.rs b/src/agent/routine.rs index 5a57a8a66b7..0383b1571cf 100644 --- a/src/agent/routine.rs +++ b/src/agent/routine.rs @@ -24,6 +24,8 @@ use std::time::Duration; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; +use serde_json::{Map, Value}; +use sha2::{Digest, Sha256}; use uuid::Uuid; use crate::error::RoutineError; @@ -52,6 +54,55 @@ pub struct Routine { pub updated_at: DateTime, } +const ROUTINE_VERIFICATION_STATE_KEY: &str = "_verification"; + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct RoutineVerificationRecord { + current_fingerprint: String, + #[serde(default)] + verified_fingerprint: Option, + #[serde(default)] + last_verified_at: Option>, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RoutineVerificationStatus { + Verified, + Unverified, +} + +impl RoutineVerificationStatus { + pub fn as_str(self) -> &'static str { + match self { + RoutineVerificationStatus::Verified => "verified", + RoutineVerificationStatus::Unverified => "unverified", + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RoutineDisplayStatus { + Disabled, + Running, + Unverified, + Failing, + Attention, + Active, +} + +impl RoutineDisplayStatus { + pub fn as_str(self) -> &'static str { + match self { + RoutineDisplayStatus::Disabled => "disabled", + RoutineDisplayStatus::Running => "running", + RoutineDisplayStatus::Unverified => "unverified", + RoutineDisplayStatus::Failing => "failing", + RoutineDisplayStatus::Attention => "attention", + RoutineDisplayStatus::Active => "active", + } + } +} + /// When a routine should fire. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "snake_case")] @@ -522,6 +573,155 @@ pub fn content_hash(content: &str) -> u64 { hasher.finish() } +fn routine_state_as_object(state: &Value) -> Map { + state.as_object().cloned().unwrap_or_default() +} + +fn routine_verification_record(state: &Value) -> Option { + state + .as_object() + .and_then(|obj| obj.get(ROUTINE_VERIFICATION_STATE_KEY)) + .cloned() + .and_then(|value| serde_json::from_value(value).ok()) +} + +fn write_routine_verification_record( + state: &Value, + record: RoutineVerificationRecord, +) -> serde_json::Value { + let mut obj = routine_state_as_object(state); + if let Ok(value) = serde_json::to_value(record) { + obj.insert(ROUTINE_VERIFICATION_STATE_KEY.to_string(), value); + } + Value::Object(obj) +} + +fn canonicalize_json_value(value: Value) -> Value { + match value { + Value::Array(items) => { + Value::Array(items.into_iter().map(canonicalize_json_value).collect()) + } + Value::Object(obj) => { + let mut keys: Vec = obj.keys().cloned().collect(); + keys.sort(); + let mut canonical = Map::new(); + for key in keys { + if let Some(value) = obj.get(&key) { + canonical.insert(key, canonicalize_json_value(value.clone())); + } + } + Value::Object(canonical) + } + other => other, + } +} + +pub fn routine_verification_fingerprint(routine: &Routine) -> String { + let canonical = canonicalize_json_value(serde_json::json!({ + "trigger_type": routine.trigger.type_tag(), + "trigger": routine.trigger.to_config_json(), + "action_type": routine.action.type_tag(), + "action": routine.action.to_config_json(), + "guardrails": { + "cooldown_secs": routine.guardrails.cooldown.as_secs(), + "max_concurrent": routine.guardrails.max_concurrent, + "dedup_window_secs": routine.guardrails.dedup_window.map(|d| d.as_secs()), + }, + })) + .to_string(); + let mut hasher = Sha256::new(); + hasher.update(canonical.as_bytes()); + hex::encode(hasher.finalize()) +} + +pub fn reset_routine_verification_state( + state: &Value, + current_fingerprint: String, +) -> serde_json::Value { + let mut record = routine_verification_record(state).unwrap_or(RoutineVerificationRecord { + current_fingerprint: current_fingerprint.clone(), + verified_fingerprint: None, + last_verified_at: None, + }); + record.current_fingerprint = current_fingerprint; + write_routine_verification_record(state, record) +} + +pub fn apply_routine_verification_result( + state: &Value, + current_fingerprint: String, + status: RunStatus, + now: DateTime, +) -> serde_json::Value { + if let Some(mut record) = routine_verification_record(state) { + record.current_fingerprint = current_fingerprint.clone(); + if status == RunStatus::Ok { + record.verified_fingerprint = Some(current_fingerprint); + record.last_verified_at = Some(now); + } + write_routine_verification_record(state, record) + } else if status == RunStatus::Ok { + write_routine_verification_record( + state, + RoutineVerificationRecord { + current_fingerprint: current_fingerprint.clone(), + verified_fingerprint: Some(current_fingerprint), + last_verified_at: Some(now), + }, + ) + } else { + state.clone() + } +} + +pub fn routine_verification_status(routine: &Routine) -> RoutineVerificationStatus { + let fingerprint = routine_verification_fingerprint(routine); + let verified = + routine_verification_record(&routine.state).map_or(routine.run_count > 0, |record| { + record.current_fingerprint == fingerprint + && record.verified_fingerprint.as_deref() == Some(fingerprint.as_str()) + }); + if verified { + RoutineVerificationStatus::Verified + } else { + RoutineVerificationStatus::Unverified + } +} + +pub fn routine_display_status( + routine: &Routine, + last_run_status: Option, +) -> RoutineDisplayStatus { + routine_display_status_for_verification( + routine, + routine_verification_status(routine), + last_run_status, + ) +} + +pub fn routine_display_status_for_verification( + routine: &Routine, + verification_status: RoutineVerificationStatus, + last_run_status: Option, +) -> RoutineDisplayStatus { + if !routine.enabled { + return RoutineDisplayStatus::Disabled; + } + if last_run_status == Some(RunStatus::Running) { + return RoutineDisplayStatus::Running; + } + if verification_status == RoutineVerificationStatus::Unverified { + return RoutineDisplayStatus::Unverified; + } + if routine.consecutive_failures > 0 { + return RoutineDisplayStatus::Failing; + } + if last_run_status == Some(RunStatus::Attention) { + return RoutineDisplayStatus::Attention; + } + RoutineDisplayStatus::Active +} + /// Normalize a cron expression to the 7-field format expected by the `cron` crate. /// /// The `cron` crate requires: `sec min hour day-of-month month day-of-week year`. @@ -730,9 +930,14 @@ pub fn describe_cron(schedule: &str, timezone: Option<&str>) -> String { #[cfg(test)] mod tests { use crate::agent::routine::{ - MAX_TOOL_ROUNDS_LIMIT, RoutineAction, RoutineGuardrails, RunStatus, Trigger, content_hash, - describe_cron, next_cron_fire, normalize_cron_expression, + MAX_TOOL_ROUNDS_LIMIT, NotifyConfig, Routine, RoutineAction, RoutineGuardrails, + RoutineVerificationStatus, RunStatus, Trigger, apply_routine_verification_result, + content_hash, describe_cron, next_cron_fire, normalize_cron_expression, + reset_routine_verification_state, routine_verification_fingerprint, + routine_verification_status, }; + use chrono::Utc; + use uuid::Uuid; #[test] fn test_trigger_roundtrip() { @@ -866,6 +1071,69 @@ mod tests { assert_ne!(h1, h3); } + #[test] + fn test_verification_fingerprint_is_digest_not_prompt_content() { + let routine = Routine { + id: Uuid::new_v4(), + name: "hashed".to_string(), + description: "hash test".to_string(), + user_id: "test-user".to_string(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::Lightweight { + prompt: "super-secret-routine-prompt".to_string(), + context_paths: Vec::new(), + max_tokens: 256, + use_tools: false, + max_tool_rounds: 1, + }, + guardrails: RoutineGuardrails::default(), + notify: NotifyConfig::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(), + }; + + let fingerprint = routine_verification_fingerprint(&routine); + + assert_eq!(fingerprint.len(), 64); + assert!(!fingerprint.contains("super-secret-routine-prompt")); + } + + #[test] + fn test_system_event_fingerprint_is_stable_when_filter_insertion_order_differs() { + let mut first_filters = std::collections::HashMap::new(); + first_filters.insert("repo".to_string(), "nearai/ironclaw".to_string()); + first_filters.insert("action".to_string(), "opened".to_string()); + + let mut second_filters = std::collections::HashMap::new(); + second_filters.insert("action".to_string(), "opened".to_string()); + second_filters.insert("repo".to_string(), "nearai/ironclaw".to_string()); + + let mut first = make_verification_test_routine(); + first.trigger = Trigger::SystemEvent { + source: "github".to_string(), + event_type: "issue".to_string(), + filters: first_filters, + }; + + let mut second = make_verification_test_routine(); + second.trigger = Trigger::SystemEvent { + source: "github".to_string(), + event_type: "issue".to_string(), + filters: second_filters, + }; + + assert_eq!( + routine_verification_fingerprint(&first), + routine_verification_fingerprint(&second) + ); + } + #[test] fn test_next_cron_fire_valid() { // Every minute should always have a next fire @@ -1122,4 +1390,173 @@ mod tests { _ => panic!("expected Lightweight"), } } + + fn make_verification_test_routine() -> Routine { + Routine { + id: Uuid::new_v4(), + name: "verify-me".to_string(), + description: "verification test".to_string(), + user_id: "test-user".to_string(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::Lightweight { + prompt: "Check routine output".to_string(), + context_paths: Vec::new(), + max_tokens: 1024, + use_tools: false, + max_tool_rounds: 1, + }, + guardrails: RoutineGuardrails::default(), + notify: NotifyConfig::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(), + } + } + + #[test] + fn test_reset_verification_state_marks_new_routine_unverified() { + let mut routine = make_verification_test_routine(); + routine.state = reset_routine_verification_state( + &routine.state, + routine_verification_fingerprint(&routine), + ); + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Unverified + ); + } + + #[test] + fn test_successful_run_verifies_current_fingerprint() { + let mut routine = make_verification_test_routine(); + let fingerprint = routine_verification_fingerprint(&routine); + routine.state = reset_routine_verification_state(&routine.state, fingerprint.clone()); + routine.state = apply_routine_verification_result( + &routine.state, + fingerprint, + RunStatus::Ok, + Utc::now(), + ); + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Verified + ); + } + + #[test] + fn test_behavior_change_resets_prior_verification() { + let mut routine = make_verification_test_routine(); + let original_fingerprint = routine_verification_fingerprint(&routine); + routine.state = + reset_routine_verification_state(&routine.state, original_fingerprint.clone()); + routine.state = apply_routine_verification_result( + &routine.state, + original_fingerprint, + RunStatus::Ok, + Utc::now(), + ); + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Verified + ); + + if let RoutineAction::Lightweight { prompt, .. } = &mut routine.action { + *prompt = "Updated prompt".to_string(); + } + routine.state = reset_routine_verification_state( + &routine.state, + routine_verification_fingerprint(&routine), + ); + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Unverified + ); + } + + #[test] + fn test_failed_unverified_run_stays_unverified() { + let mut routine = make_verification_test_routine(); + let fingerprint = routine_verification_fingerprint(&routine); + routine.state = reset_routine_verification_state(&routine.state, fingerprint.clone()); + routine.state = apply_routine_verification_result( + &routine.state, + fingerprint, + RunStatus::Failed, + Utc::now(), + ); + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Unverified + ); + } + + #[test] + fn test_schedule_change_resets_verification() { + let mut routine = make_verification_test_routine(); + routine.trigger = Trigger::Cron { + schedule: "0 0 9 * * MON-FRI *".to_string(), + timezone: Some("UTC".to_string()), + }; + let original_fingerprint = routine_verification_fingerprint(&routine); + routine.state = + reset_routine_verification_state(&routine.state, original_fingerprint.clone()); + routine.state = apply_routine_verification_result( + &routine.state, + original_fingerprint, + RunStatus::Ok, + Utc::now(), + ); + + routine.trigger = Trigger::Cron { + schedule: "0 0 10 * * MON-FRI *".to_string(), + timezone: Some("UTC".to_string()), + }; + routine.state = reset_routine_verification_state( + &routine.state, + routine_verification_fingerprint(&routine), + ); + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Unverified + ); + } + + #[test] + fn test_legacy_routine_with_runs_is_treated_as_verified_without_metadata() { + let mut routine = make_verification_test_routine(); + routine.run_count = 3; + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Verified + ); + } + + #[test] + fn test_failed_legacy_run_preserves_implicit_verification() { + let mut routine = make_verification_test_routine(); + routine.run_count = 2; + let fingerprint = routine_verification_fingerprint(&routine); + routine.state = apply_routine_verification_result( + &routine.state, + fingerprint, + RunStatus::Failed, + Utc::now(), + ); + + assert_eq!( + routine_verification_status(&routine), + RoutineVerificationStatus::Verified + ); + } } diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 65682e494cb..410cc69d56d 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -23,7 +23,8 @@ use uuid::Uuid; use crate::agent::Scheduler; use crate::agent::routine::{ - NotifyConfig, Routine, RoutineAction, RoutineRun, RunStatus, Trigger, next_cron_fire, + NotifyConfig, Routine, RoutineAction, RoutineRun, RunStatus, Trigger, + apply_routine_verification_result, next_cron_fire, routine_verification_fingerprint, }; use crate::channels::{IncomingMessage, OutgoingResponse}; use crate::config::RoutineConfig; @@ -621,7 +622,7 @@ impl RoutineEngine { ); // Load the routine to update consecutive_failures and send notification - let routine = match self.store.get_routine(run.routine_id).await { + let mut routine = match self.store.get_routine(run.routine_id).await { Ok(Some(r)) => r, Ok(None) => { tracing::warn!( @@ -649,6 +650,12 @@ impl RoutineEngine { }; let now = Utc::now(); + routine.state = apply_routine_verification_result( + &routine.state, + routine_verification_fingerprint(&routine), + status, + now, + ); let next_fire = if let Trigger::Cron { ref schedule, ref timezone, @@ -1085,7 +1092,7 @@ struct EngineContext { } /// Execute a routine run. Handles both lightweight and full_job modes. -async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) { +async fn execute_routine(ctx: EngineContext, mut routine: Routine, run: RoutineRun) { // Increment running count (atomic: survives panics in the execution below) ctx.running_count.fetch_add(1, Ordering::Relaxed); @@ -1143,8 +1150,15 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) tracing::error!(routine = %routine.name, "Failed to complete run record: {}", e); } - // Update routine runtime state let now = Utc::now(); + routine.state = apply_routine_verification_result( + &routine.state, + routine_verification_fingerprint(&routine), + status, + now, + ); + + // Update routine runtime state let next_fire = if let Trigger::Cron { ref schedule, ref timezone, diff --git a/src/channels/web/CLAUDE.md b/src/channels/web/CLAUDE.md index 28fc5d55994..1bc56b76b95 100644 --- a/src/channels/web/CLAUDE.md +++ b/src/channels/web/CLAUDE.md @@ -84,7 +84,7 @@ Browser-facing HTTP API and SSE/WebSocket real-time streaming. Axum-based, singl | Method | Path | Description | |--------|------|-------------| | GET | `/api/routines` | List routines | -| GET | `/api/routines/summary` | Aggregated stats (total/enabled/disabled/failing/runs_today) | +| GET | `/api/routines/summary` | Aggregated stats (total/enabled/disabled/unverified/failing/runs_today) | | GET | `/api/routines/{id}` | Routine detail with recent run history | | POST | `/api/routines/{id}/trigger` | Manually trigger a routine | | POST | `/api/routines/{id}/toggle` | Enable/disable a routine | diff --git a/src/channels/web/handlers/routines.rs b/src/channels/web/handlers/routines.rs index ebbb599eff9..5cfcc864dbe 100644 --- a/src/channels/web/handlers/routines.rs +++ b/src/channels/web/handlers/routines.rs @@ -10,7 +10,10 @@ use axum::{ use serde::Deserialize; use uuid::Uuid; -use crate::agent::routine::{Trigger, next_cron_fire}; +use crate::agent::routine::{ + RoutineDisplayStatus, RoutineVerificationStatus, Trigger, next_cron_fire, + routine_display_status_for_verification, routine_verification_status, +}; use crate::channels::web::auth::AuthenticatedUser; use crate::channels::web::server::GatewayState; use crate::channels::web::types::*; @@ -30,7 +33,18 @@ pub async fn routines_list_handler( .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; - let items: Vec = routines.iter().map(RoutineInfo::from_routine).collect(); + let routine_ids: Vec = routines.iter().map(|routine| routine.id).collect(); + let last_run_statuses = store + .batch_get_last_run_status(&routine_ids) + .await + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + + let items: Vec = routines + .iter() + .map(|routine| { + RoutineInfo::from_routine(routine, last_run_statuses.get(&routine.id).copied()) + }) + .collect(); Ok(Json(RoutineListResponse { routines: items })) } @@ -49,13 +63,39 @@ pub async fn routines_summary_handler( .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + let routine_ids: Vec = routines.iter().map(|routine| routine.id).collect(); + let last_run_statuses = store + .batch_get_last_run_status(&routine_ids) + .await + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + let total = routines.len() as u64; - let enabled = routines.iter().filter(|r| r.enabled).count() as u64; - let disabled = total - enabled; - let failing = routines - .iter() - .filter(|r| r.consecutive_failures > 0) - .count() as u64; + let mut enabled = 0u64; + let mut disabled = 0u64; + let mut unverified = 0u64; + let mut failing = 0u64; + + for routine in &routines { + let verification_status = routine_verification_status(routine); + if routine.enabled { + enabled += 1; + } else { + disabled += 1; + } + + if verification_status == RoutineVerificationStatus::Unverified { + unverified += 1; + } + + if routine_display_status_for_verification( + routine, + verification_status, + last_run_statuses.get(&routine.id).copied(), + ) == RoutineDisplayStatus::Failing + { + failing += 1; + } + } let today_start = chrono::Utc::now() .date_naive() @@ -74,6 +114,7 @@ pub async fn routines_summary_handler( total, enabled, disabled, + unverified, failing, runs_today, })) @@ -120,7 +161,7 @@ pub async fn routines_detail_handler( job_id: run.job_id, }) .collect(); - let routine_info = RoutineInfo::from_routine(&routine); + let routine_info = RoutineInfo::from_routine(&routine, runs.first().map(|run| run.status)); // Read-only lookup — do not create a conversation on a GET request. // The conversation is created lazily when the routine first executes. @@ -148,6 +189,8 @@ pub async fn routines_detail_handler( next_fire_at: routine.next_fire_at.map(|dt| dt.to_rfc3339()), run_count: routine.run_count, consecutive_failures: routine.consecutive_failures, + status: routine_info.status.clone(), + verification_status: routine_info.verification_status.clone(), created_at: routine.created_at.to_rfc3339(), conversation_id, recent_runs, diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index c0df6e78f41..710c5a587fc 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -1014,7 +1014,42 @@ async fn oauth_callback_handler( flow.secrets .create(&flow.user_id, params) .await - .map_err(|e| e.to_string())?; + .map_err(|e| { + tracing::warn!( + extension = %flow.extension_name, + secret_name = %client_id_secret, + error = %e, + "Failed to store OAuth client_id secret after callback" + ); + "failed to store client credentials".to_string() + })?; + } + + if let (Some(client_secret_name), Some(client_secret)) = ( + flow.client_secret_secret_name.as_ref(), + flow.client_secret.as_deref(), + ) { + let mut params = + crate::secrets::CreateSecretParams::new(client_secret_name, client_secret) + .with_provider(flow.provider.as_ref().cloned().unwrap_or_default()); + if let Some(expires_at) = flow.client_secret_expires_at + && let Some(dt) = + chrono::DateTime::::from_timestamp(expires_at as i64, 0) + { + params = params.with_expiry(dt); + } + flow.secrets + .create(&flow.user_id, params) + .await + .map_err(|e| { + tracing::warn!( + extension = %flow.extension_name, + secret_name = %client_secret_name, + error = %e, + "Failed to store OAuth client_secret secret after callback" + ); + "failed to store client credentials".to_string() + })?; } Ok(()) @@ -3177,6 +3212,8 @@ mod tests { gateway_token: oauth_proxy_auth_token, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at: std::time::Instant::now(), } } @@ -3542,6 +3579,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at, }; @@ -3611,6 +3650,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at, }; @@ -3714,6 +3755,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, // Expired — handler will reject after lookup (no network I/O) created_at, }; @@ -3801,6 +3844,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at, }; @@ -3882,6 +3927,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at, }; diff --git a/src/channels/web/static/app.js b/src/channels/web/static/app.js index 2f4b972fc1e..8fa26a3418c 100644 --- a/src/channels/web/static/app.js +++ b/src/channels/web/static/app.js @@ -4182,6 +4182,7 @@ function renderRoutinesSummary(s) { + summaryCard(I18n.t('routines.summary.total'), s.total, '') + summaryCard(I18n.t('routines.summary.enabled'), s.enabled, 'active') + summaryCard(I18n.t('routines.summary.disabled'), s.disabled, '') + + summaryCard(I18n.t('routines.summary.unverified'), s.unverified, 'pending') + summaryCard(I18n.t('routines.summary.failing'), s.failing, 'failed') + summaryCard(I18n.t('routines.summary.runsToday'), s.runs_today, 'completed'); } @@ -4200,6 +4201,8 @@ function renderRoutinesList(routines) { tbody.innerHTML = routines.map((r) => { const statusClass = r.status === 'active' ? 'completed' : r.status === 'failing' ? 'failed' + : r.status === 'attention' ? 'stuck' + : r.status === 'running' ? 'in_progress' : 'pending'; const toggleLabel = r.enabled ? 'Disable' : 'Enable'; @@ -4207,6 +4210,9 @@ function renderRoutinesList(routines) { const triggerTitle = (r.trigger_type === 'cron' && r.trigger_raw) ? ' title="' + escapeHtml(r.trigger_raw) + '"' : ''; + const runLabel = (r.verification_status === 'unverified' || r.status === 'unverified') + ? 'Verify now' + : 'Run'; return '' + '' + escapeHtml(r.name) + '' @@ -4218,7 +4224,7 @@ function renderRoutinesList(routines) { + '' + escapeHtml(r.status) + '' + '' + ' ' - + ' ' + + ' ' + '' + '' + ''; @@ -4247,12 +4253,12 @@ function renderRoutineDetail(routine) { const detail = document.getElementById('routine-detail'); detail.style.display = 'block'; - const statusClass = !routine.enabled ? 'pending' - : routine.consecutive_failures > 0 ? 'failed' - : 'completed'; - const statusLabel = !routine.enabled ? 'disabled' - : routine.consecutive_failures > 0 ? 'failing' - : 'active'; + const statusClass = routine.status === 'active' ? 'completed' + : routine.status === 'failing' ? 'failed' + : routine.status === 'attention' ? 'stuck' + : routine.status === 'running' ? 'in_progress' + : 'pending'; + const statusLabel = routine.status || 'active'; let html = '
' + '' @@ -4277,6 +4283,20 @@ function renderRoutineDetail(routine) { + '
' + escapeHtml(routine.description) + '
'; } + if (routine.verification_status === 'unverified') { + let verificationCopy = 'Created or updated, but not yet verified with a successful run.'; + if (routine.recent_runs && routine.recent_runs.length > 0) { + const latestRun = routine.recent_runs[0]; + if (latestRun.status === 'failed') { + verificationCopy = 'The latest verification attempt failed. Review the run details and verify again after fixing it.'; + } else if (latestRun.status === 'attention') { + verificationCopy = 'The latest verification attempt needs attention. Review the run details and verify again when ready.'; + } + } + html += '

Verification

' + + '
' + escapeHtml(verificationCopy) + '
'; + } + // Trigger config if (routine.trigger_type === 'cron') { const summary = routine.trigger_summary || 'cron'; diff --git a/src/channels/web/static/i18n/en.js b/src/channels/web/static/i18n/en.js index ea06d9d549d..a1ca3e9724f 100644 --- a/src/channels/web/static/i18n/en.js +++ b/src/channels/web/static/i18n/en.js @@ -246,6 +246,7 @@ I18n.register('en', { 'routines.summary.total': 'Total', 'routines.summary.enabled': 'Enabled', 'routines.summary.disabled': 'Disabled', + 'routines.summary.unverified': 'Unverified', 'routines.summary.failing': 'Failing', 'routines.summary.runsToday': 'Runs Today', diff --git a/src/channels/web/static/i18n/zh-CN.js b/src/channels/web/static/i18n/zh-CN.js index 800e697bb14..b0f25205c40 100644 --- a/src/channels/web/static/i18n/zh-CN.js +++ b/src/channels/web/static/i18n/zh-CN.js @@ -246,6 +246,7 @@ I18n.register('zh-CN', { 'routines.summary.total': '总计', 'routines.summary.enabled': '已启用', 'routines.summary.disabled': '已禁用', + 'routines.summary.unverified': '未验证', 'routines.summary.failing': '失败', 'routines.summary.runsToday': '今日运行', diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index 9ecece5725e..584621003c9 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -662,11 +662,15 @@ pub struct RoutineInfo { pub run_count: u64, pub consecutive_failures: u32, pub status: String, + pub verification_status: String, } impl RoutineInfo { /// Convert a `Routine` to the trimmed `RoutineInfo` for list display. - pub fn from_routine(r: &crate::agent::routine::Routine) -> Self { + pub fn from_routine( + r: &crate::agent::routine::Routine, + last_run_status: Option, + ) -> Self { let (trigger_type, trigger_raw, trigger_summary) = match &r.trigger { crate::agent::routine::Trigger::Cron { schedule, timezone } => ( "cron".to_string(), @@ -710,13 +714,13 @@ impl RoutineInfo { crate::agent::routine::RoutineAction::FullJob { .. } => "full_job", }; - let status = if !r.enabled { - "disabled" - } else if r.consecutive_failures > 0 { - "failing" - } else { - "active" - }; + let verification_status = crate::agent::routine::routine_verification_status(r); + let status = crate::agent::routine::routine_display_status_for_verification( + r, + verification_status, + last_run_status, + ) + .as_str(); RoutineInfo { id: r.id, @@ -732,6 +736,7 @@ impl RoutineInfo { run_count: r.run_count, consecutive_failures: r.consecutive_failures, status: status.to_string(), + verification_status: verification_status.as_str().to_string(), } } } @@ -746,6 +751,7 @@ pub struct RoutineSummaryResponse { pub total: u64, pub enabled: u64, pub disabled: u64, + pub unverified: u64, pub failing: u64, pub runs_today: u64, } @@ -767,6 +773,8 @@ pub struct RoutineDetailResponse { pub next_fire_at: Option, pub run_count: u64, pub consecutive_failures: u32, + pub status: String, + pub verification_status: String, pub created_at: String, pub conversation_id: Option, pub recent_runs: Vec, @@ -824,6 +832,7 @@ pub struct HealthResponse { #[cfg(test)] mod tests { use super::*; + use chrono::Utc; // ---- WsClientMessage deserialization tests ---- @@ -1174,4 +1183,130 @@ mod tests { let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); assert!(parsed.get("channel").is_none()); } + + fn make_routine_for_status_tests() -> crate::agent::routine::Routine { + crate::agent::routine::Routine { + id: Uuid::new_v4(), + name: "status-check".to_string(), + description: "routine status test".to_string(), + user_id: "test-user".to_string(), + enabled: true, + trigger: crate::agent::routine::Trigger::Manual, + action: crate::agent::routine::RoutineAction::Lightweight { + prompt: "Check status".to_string(), + context_paths: Vec::new(), + max_tokens: 256, + use_tools: false, + max_tool_rounds: 1, + }, + guardrails: crate::agent::routine::RoutineGuardrails::default(), + notify: crate::agent::routine::NotifyConfig::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(), + } + } + + #[test] + fn test_routine_info_marks_new_routine_unverified() { + let mut routine = make_routine_for_status_tests(); + routine.state = crate::agent::routine::reset_routine_verification_state( + &routine.state, + crate::agent::routine::routine_verification_fingerprint(&routine), + ); + + let info = RoutineInfo::from_routine(&routine, None); + + assert_eq!(info.status, "unverified"); + assert_eq!(info.verification_status, "unverified"); + } + + #[test] + fn test_routine_info_preserves_verified_state_for_description_only_changes() { + let mut routine = make_routine_for_status_tests(); + let fingerprint = crate::agent::routine::routine_verification_fingerprint(&routine); + routine.state = crate::agent::routine::reset_routine_verification_state( + &routine.state, + fingerprint.clone(), + ); + routine.state = crate::agent::routine::apply_routine_verification_result( + &routine.state, + fingerprint, + crate::agent::routine::RunStatus::Ok, + Utc::now(), + ); + routine.description = "Updated description".to_string(); + + let info = RoutineInfo::from_routine(&routine, Some(crate::agent::routine::RunStatus::Ok)); + + assert_eq!(info.status, "active"); + assert_eq!(info.verification_status, "verified"); + } + + #[test] + fn test_routine_info_surfaces_running_before_unverified() { + let mut routine = make_routine_for_status_tests(); + routine.state = crate::agent::routine::reset_routine_verification_state( + &routine.state, + crate::agent::routine::routine_verification_fingerprint(&routine), + ); + + let info = + RoutineInfo::from_routine(&routine, Some(crate::agent::routine::RunStatus::Running)); + + assert_eq!(info.status, "running"); + assert_eq!(info.verification_status, "unverified"); + } + + #[test] + fn test_routine_info_keeps_verified_state_when_disabled() { + let mut routine = make_routine_for_status_tests(); + let fingerprint = crate::agent::routine::routine_verification_fingerprint(&routine); + routine.state = crate::agent::routine::reset_routine_verification_state( + &routine.state, + fingerprint.clone(), + ); + routine.state = crate::agent::routine::apply_routine_verification_result( + &routine.state, + fingerprint, + crate::agent::routine::RunStatus::Ok, + Utc::now(), + ); + routine.enabled = false; + + let info = RoutineInfo::from_routine(&routine, Some(crate::agent::routine::RunStatus::Ok)); + + assert_eq!(info.status, "disabled"); + assert_eq!(info.verification_status, "verified"); + } + + #[test] + fn test_routine_info_treats_legacy_run_history_as_verified() { + let mut routine = make_routine_for_status_tests(); + routine.run_count = 2; + + let info = RoutineInfo::from_routine(&routine, Some(crate::agent::routine::RunStatus::Ok)); + + assert_eq!(info.status, "active"); + assert_eq!(info.verification_status, "verified"); + } + + #[test] + fn test_routine_info_keeps_unverified_state_when_disabled() { + let mut routine = make_routine_for_status_tests(); + routine.state = crate::agent::routine::reset_routine_verification_state( + &routine.state, + crate::agent::routine::routine_verification_fingerprint(&routine), + ); + routine.enabled = false; + + let info = RoutineInfo::from_routine(&routine, None); + + assert_eq!(info.status, "disabled"); + assert_eq!(info.verification_status, "unverified"); + } } diff --git a/src/cli/oauth_defaults.rs b/src/cli/oauth_defaults.rs index 5628f3d689b..2008afe0c20 100644 --- a/src/cli/oauth_defaults.rs +++ b/src/cli/oauth_defaults.rs @@ -102,6 +102,8 @@ pub struct OAuthTokenResponse { pub access_token: String, pub refresh_token: Option, pub expires_in: Option, + pub token_type: Option, + pub scope: Option, } /// Result of building an OAuth 2.0 authorization URL. @@ -294,6 +296,14 @@ pub async fn exchange_oauth_code_with_params( access_token, refresh_token, expires_in, + token_type: token_data + .get("token_type") + .and_then(|v| v.as_str()) + .map(String::from), + scope: token_data + .get("scope") + .and_then(|v| v.as_str()) + .map(String::from), }) } @@ -482,6 +492,12 @@ pub struct PendingOAuthFlow { /// Secret name for persisting the client ID (MCP OAuth only). /// Needed so token refresh can find the client_id after the session ends. pub client_id_secret_name: Option, + /// Secret name for persisting the client secret (MCP DCR only). + /// Needed for providers that return a client secret during DCR and expect + /// it to be replayed during later refreshes. + pub client_secret_secret_name: Option, + /// Absolute UNIX timestamp when the DCR client secret expires, if any. + pub client_secret_expires_at: Option, /// When this flow was created (for expiry). pub created_at: std::time::Instant, } @@ -769,6 +785,7 @@ pub struct ProxyRefreshTokenRequest<'a> { pub client_id: &'a str, pub client_secret: Option<&'a str>, pub refresh_token: &'a str, + pub resource: Option<&'a str>, pub provider: Option<&'a str>, } @@ -801,6 +818,14 @@ fn oauth_token_response_from_json( access_token, refresh_token, expires_in, + token_type: token_data + .get("token_type") + .and_then(|v| v.as_str()) + .map(String::from), + scope: token_data + .get("scope") + .and_then(|v| v.as_str()) + .map(String::from), }) } @@ -900,6 +925,9 @@ pub async fn refresh_token_via_proxy( if let Some(secret) = request.client_secret { params.push(("client_secret", secret.to_string())); } + if let Some(resource) = request.resource { + params.push(("resource", resource.to_string())); + } if let Some(provider) = request.provider { params.push(("provider", provider.to_string())); } @@ -988,8 +1016,10 @@ mod tests { }); Json(json!({ "access_token": "proxy-access-token", + "token_type": "Bearer", "refresh_token": "proxy-refresh-token", - "expires_in": 7200 + "expires_in": 7200, + "scope": "scope-a scope-b" })) } @@ -1007,8 +1037,10 @@ mod tests { }); Json(json!({ "access_token": "proxy-access-token", + "token_type": "Bearer", "refresh_token": "proxy-refresh-token", - "expires_in": 7200 + "expires_in": 7200, + "scope": "scope-a scope-b" })) } @@ -1165,6 +1197,8 @@ mod tests { Some("proxy-refresh-token") ); assert_eq!(response.expires_in, Some(7200)); + assert_eq!(response.token_type.as_deref(), Some("Bearer")); + assert_eq!(response.scope.as_deref(), Some("scope-a scope-b")); let requests = server.requests().await; assert_eq!(requests.len(), 1); @@ -1222,6 +1256,7 @@ mod tests { client_id: TEST_OAUTH_CLIENT_ID, client_secret: Some(TEST_OAUTH_CLIENT_SECRET), refresh_token: "refresh-token-123", + resource: Some("https://mcp.notion.com"), provider: Some("google"), }) .await @@ -1233,6 +1268,8 @@ mod tests { Some("proxy-refresh-token") ); assert_eq!(response.expires_in, Some(7200)); + assert_eq!(response.token_type.as_deref(), Some("Bearer")); + assert_eq!(response.scope.as_deref(), Some("scope-a scope-b")); let requests = server.requests().await; assert_eq!(requests.len(), 1); @@ -1260,6 +1297,10 @@ mod tests { requests[0].form.get("provider").map(String::as_str), Some("google") ); + assert_eq!( + requests[0].form.get("resource").map(String::as_str), + Some("https://mcp.notion.com") + ); server.shutdown().await; } @@ -1303,6 +1344,7 @@ mod tests { client_id: TEST_OAUTH_CLIENT_ID, client_secret: Some(TEST_OAUTH_CLIENT_SECRET), refresh_token: "refresh-token-123", + resource: Some("https://mcp.notion.com"), provider: Some("google"), }) .await diff --git a/src/db/libsql/routines.rs b/src/db/libsql/routines.rs index 504d77dca56..973e3fcd111 100644 --- a/src/db/libsql/routines.rs +++ b/src/db/libsql/routines.rs @@ -4,7 +4,7 @@ use std::collections::{HashMap, HashSet}; use async_trait::async_trait; use chrono::{DateTime, Utc}; -use libsql::params; +use libsql::{params, params_from_iter}; use uuid::Uuid; use super::{ @@ -471,25 +471,33 @@ impl RoutineStore for LibSqlBackend { } 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 requested_rows = (1..=routine_ids.len()) + .map(|i| format!("(?{i})")) + .collect::>() + .join(", "); + let requested_ids = routine_ids + .iter() + .map(|id| id.to_string()) + .collect::>(); + let sql = format!( + "WITH requested(routine_id) AS (VALUES {requested_rows}) + SELECT r1.routine_id, r1.status + FROM routine_runs r1 + JOIN ( + SELECT rr.routine_id, MAX(rr.started_at) AS max_started_at + FROM routine_runs rr + JOIN requested req ON req.routine_id = rr.routine_id + GROUP BY rr.routine_id + ) latest + ON latest.routine_id = r1.routine_id + AND latest.max_started_at = r1.started_at" + ); 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![], - ) + .query(&sql, params_from_iter(requested_ids)) .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 @@ -501,11 +509,9 @@ impl RoutineStore for LibSqlBackend { 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); - } + let status_str: String = get_text(&row, 1); + if let std::result::Result::Ok(status) = status_str.parse::() { + statuses.insert(id, status); } } @@ -594,3 +600,91 @@ impl RoutineStore for LibSqlBackend { Ok(runs) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::agent::routine::{ + NotifyConfig, Routine, RoutineAction, RoutineGuardrails, RoutineRun, Trigger, + }; + use crate::db::{Database, RoutineStore}; + + fn test_routine(user_id: &str, name: &str) -> Routine { + Routine { + id: Uuid::new_v4(), + name: name.to_string(), + description: "test routine".to_string(), + user_id: user_id.to_string(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::Lightweight { + prompt: "test".to_string(), + context_paths: Vec::new(), + max_tokens: 128, + use_tools: false, + max_tool_rounds: 1, + }, + guardrails: RoutineGuardrails::default(), + notify: NotifyConfig::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 test_run(routine_id: Uuid, status: RunStatus, started_at: DateTime) -> RoutineRun { + RoutineRun { + id: Uuid::new_v4(), + routine_id, + trigger_type: "manual".to_string(), + trigger_detail: None, + started_at, + completed_at: None, + status, + result_summary: None, + tokens_used: None, + job_id: None, + created_at: started_at, + } + } + + #[tokio::test] + async fn batch_get_last_run_status_is_scoped_to_requested_routines() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("routine-status.db"); + let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); + backend.run_migrations().await.unwrap(); + + let requested = test_routine("user-1", "requested"); + let other = test_routine("user-1", "other"); + backend.create_routine(&requested).await.unwrap(); + backend.create_routine(&other).await.unwrap(); + + let now = Utc::now(); + backend + .create_routine_run(&test_run(requested.id, RunStatus::Ok, now)) + .await + .unwrap(); + backend + .create_routine_run(&test_run( + other.id, + RunStatus::Failed, + now + chrono::Duration::seconds(1), + )) + .await + .unwrap(); + + let statuses = backend + .batch_get_last_run_status(&[requested.id]) + .await + .unwrap(); + + assert_eq!(statuses.len(), 1); + assert_eq!(statuses.get(&requested.id), Some(&RunStatus::Ok)); + assert!(!statuses.contains_key(&other.id)); + } +} diff --git a/src/db/libsql/users.rs b/src/db/libsql/users.rs index f9ddfda9a71..a317b4d5712 100644 --- a/src/db/libsql/users.rs +++ b/src/db/libsql/users.rs @@ -981,7 +981,7 @@ mod tests { assert!(alice_stats.last_active_at.is_some()); // Bob has no LLM calls so doesn't appear in summary stats - assert!(stats.iter().find(|s| s.user_id == "bob").is_none()); + assert!(!stats.iter().any(|s| s.user_id == "bob")); // Filter to single user let alice_only = db.user_summary_stats(Some("alice")).await.unwrap(); diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index c4d4e2ba6d0..be97971d1c5 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -2779,19 +2779,24 @@ impl ExtensionManager { }; // Try DCR if no client_id configured - let (client_id, client_secret) = if let Some(ref oauth) = server.oauth { - (oauth.client_id.clone(), None) - } else if let Some(ref reg_endpoint) = metadata.registration_endpoint { - let registration = register_client(reg_endpoint, &redirect_uri) - .await - .map_err(|e| ExtensionError::AuthFailed(e.to_string()))?; + let (client_id, client_secret, client_secret_expires_at) = + if let Some(ref oauth) = server.oauth { + (oauth.client_id.clone(), None, None) + } else if let Some(ref reg_endpoint) = metadata.registration_endpoint { + let registration = register_client(reg_endpoint, &redirect_uri) + .await + .map_err(|e| ExtensionError::AuthFailed(e.to_string()))?; - (registration.client_id, None) - } else { - return Err(ExtensionError::AuthNotSupported( - "Server doesn't support OAuth or Dynamic Client Registration".to_string(), - )); - }; + ( + registration.client_id, + registration.client_secret, + registration.client_secret_expires_at, + ) + } else { + return Err(ExtensionError::AuthNotSupported( + "Server doesn't support OAuth or Dynamic Client Registration".to_string(), + )); + }; // RFC 8707: resource parameter to scope the token to this MCP server let resource = canonical_resource_uri(&server.url); @@ -2823,6 +2828,7 @@ impl ExtensionManager { let code_verifier = oauth_result.code_verifier; if is_gateway { + let persist_client_secret = server.oauth.is_none() && client_secret.is_some(); let mut token_exchange_extra_params = HashMap::new(); token_exchange_extra_params.insert("resource".to_string(), resource.clone()); @@ -2849,6 +2855,12 @@ impl ExtensionManager { } else { None }, + client_secret_secret_name: if persist_client_secret { + Some(server.client_secret_secret_name()) + } else { + None + }, + client_secret_expires_at, created_at: std::time::Instant::now(), }; @@ -3106,6 +3118,7 @@ impl ExtensionManager { let token_secret_name = server.token_secret_name(); plan.add_base_secret(&token_secret_name); plan.add_base_secret(server.client_id_secret_name()); + plan.add_base_secret(server.client_secret_secret_name()); // MCP OAuth can persist companion secrets through two paths: // the MCP auth helper uses `mcp__refresh_token`, while the // hosted gateway callback stores companions alongside the access @@ -3614,6 +3627,8 @@ impl ExtensionManager { gateway_token: self.oauth_proxy_auth_token.clone(), token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at: std::time::Instant::now(), }; @@ -7922,6 +7937,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at: std::time::Instant::now(), }, ); @@ -7946,6 +7963,8 @@ mod tests { gateway_token: None, token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, + client_secret_secret_name: None, + client_secret_expires_at: None, created_at: std::time::Instant::now(), }, ); diff --git a/src/tools/builtin/routine.rs b/src/tools/builtin/routine.rs index 20257f16eee..0424add8f2d 100644 --- a/src/tools/builtin/routine.rs +++ b/src/tools/builtin/routine.rs @@ -20,7 +20,8 @@ use uuid::Uuid; use crate::agent::routine::{ NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, next_cron_fire, - normalize_cron_expression, + normalize_cron_expression, reset_routine_verification_state, routine_verification_fingerprint, + routine_verification_status, }; use crate::agent::routine_engine::RoutineEngine; use crate::context::JobContext; @@ -422,12 +423,29 @@ fn routine_create_tool_summary() -> ToolDiscoverySummary { "Set execution.use_tools=false to keep a new lightweight routine text-only.".into(), "Omitting delivery.user falls back to the owner's last-seen notification target.".into(), "advanced.cooldown_secs defaults to 300.".into(), + "Creating a routine only saves the configuration. It does not prove the routine can execute successfully.".into(), + "After routine_create, tell the user the routine is unverified and offer to test it now unless they asked not to.".into(), "Legacy flat aliases are still accepted for compatibility, but grouped fields are preferred.".into(), ], examples: routine_create_examples(), } } +fn verification_result_payload(routine: &Routine, verification_reset: bool) -> Value { + let verification_status = routine_verification_status(routine); + serde_json::json!({ + "verification_status": verification_status.as_str(), + "verification_reset": verification_reset, + "verification_hint": if verification_reset { + "The routine configuration changed and should be re-tested before being treated as reliable." + } else if verification_status == crate::agent::routine::RoutineVerificationStatus::Verified { + "The current routine configuration has already been verified with a successful run." + } else { + "The routine has been saved, but it has not been verified yet. Offer to test it now." + } + }) +} + fn routine_create_schema(include_compatibility_aliases: bool) -> Value { let mut schema = serde_json::json!({ "type": "object", @@ -1099,7 +1117,8 @@ impl Tool for RoutineCreateTool { fn description(&self) -> &str { "Create a new routine (scheduled or event-driven task). \ Supports cron schedules, event pattern matching, system events, and manual triggers. \ - Use this when the user wants something to happen periodically or reactively." + Use this when the user wants something to happen periodically or reactively. \ + Creation saves the routine, but does not verify that it will execute successfully." } fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement { @@ -1145,7 +1164,7 @@ impl Tool for RoutineCreateTool { None }; - let routine = Routine { + let mut routine = Routine { id: Uuid::new_v4(), name: normalized.name.clone(), description: normalized.description.clone(), @@ -1171,6 +1190,10 @@ impl Tool for RoutineCreateTool { created_at: Utc::now(), updated_at: Utc::now(), }; + routine.state = reset_routine_verification_state( + &routine.state, + routine_verification_fingerprint(&routine), + ); self.store .create_routine(&routine) @@ -1185,12 +1208,14 @@ impl Tool for RoutineCreateTool { self.engine.refresh_event_cache().await; } + let verification = verification_result_payload(&routine, false); let result = serde_json::json!({ "id": routine.id.to_string(), - "name": routine.name, + "name": routine.name.clone(), "trigger_type": routine.trigger.type_tag(), "next_fire_at": routine.next_fire_at.map(|t| t.to_rfc3339()), "status": "created", + "verification": verification, }); Ok(ToolOutput::success(result, start.elapsed())) @@ -1243,10 +1268,24 @@ impl Tool for RoutineListTool { .list_routines(&ctx.user_id) .await .map_err(|e| ToolError::ExecutionFailed(format!("failed to list routines: {e}")))?; + let routine_ids: Vec = routines.iter().map(|routine| routine.id).collect(); + let last_run_statuses = self + .store + .batch_get_last_run_status(&routine_ids) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!("failed to read routine statuses: {e}")) + })?; let list: Vec = routines .iter() .map(|r| { + let verification_status = routine_verification_status(r); + let status = crate::agent::routine::routine_display_status_for_verification( + r, + verification_status, + last_run_statuses.get(&r.id).copied(), + ); serde_json::json!({ "id": r.id.to_string(), "name": r.name, @@ -1258,6 +1297,8 @@ impl Tool for RoutineListTool { "next_fire_at": r.next_fire_at.map(|t| t.to_rfc3339()), "run_count": r.run_count, "consecutive_failures": r.consecutive_failures, + "status": status.as_str(), + "verification_status": verification_status.as_str(), }) }) .collect(); @@ -1296,7 +1337,8 @@ impl Tool for RoutineUpdateTool { fn description(&self) -> &str { "Update an existing routine. Can change prompt, description, enabled state, cron schedule/timezone, \ - Pass the routine name and only the fields you want to change. This does not convert trigger types." + Pass the routine name and only the fields you want to change. This does not convert trigger types. \ + Behavior-changing edits should leave the routine marked unverified until it is tested again." } fn parameters_schema(&self) -> serde_json::Value { @@ -1320,6 +1362,9 @@ impl Tool for RoutineUpdateTool { .map_err(|e| ToolError::ExecutionFailed(format!("DB error: {e}")))? .ok_or_else(|| ToolError::ExecutionFailed(format!("routine '{}' not found", name)))?; + let original_fingerprint = routine_verification_fingerprint(&routine); + let mut verification_reset = false; + // Apply updates if let Some(enabled) = params.get("enabled").and_then(|v| v.as_bool()) { routine.enabled = enabled; @@ -1331,8 +1376,18 @@ impl Tool for RoutineUpdateTool { if let Some(prompt) = params.get("prompt").and_then(|v| v.as_str()) { match &mut routine.action { - RoutineAction::Lightweight { prompt: p, .. } => *p = prompt.to_string(), - RoutineAction::FullJob { description: d, .. } => *d = prompt.to_string(), + RoutineAction::Lightweight { prompt: p, .. } => { + if p != prompt { + verification_reset = true; + *p = prompt.to_string(); + } + } + RoutineAction::FullJob { description: d, .. } => { + if d != prompt { + verification_reset = true; + *d = prompt.to_string(); + } + } } } @@ -1369,12 +1424,16 @@ impl Tool for RoutineUpdateTool { if let Some((old_schedule, old_tz)) = existing_cron { let effective_schedule = new_schedule.as_deref().unwrap_or(&old_schedule); - let effective_tz = new_timezone.or(old_tz); + let effective_tz = new_timezone.clone().or(old_tz.clone()); // Validate next_cron_fire(effective_schedule, effective_tz.as_deref()).map_err(|e| { ToolError::InvalidParameters(format!("invalid cron schedule: {e}")) })?; + if effective_schedule != old_schedule || effective_tz != old_tz { + verification_reset = true; + } + routine.trigger = Trigger::Cron { schedule: effective_schedule.to_string(), timezone: effective_tz.clone(), @@ -1388,6 +1447,12 @@ impl Tool for RoutineUpdateTool { } } + let updated_fingerprint = routine_verification_fingerprint(&routine); + if updated_fingerprint != original_fingerprint { + verification_reset = true; + routine.state = reset_routine_verification_state(&routine.state, updated_fingerprint); + } + self.store .update_routine(&routine) .await @@ -1396,12 +1461,14 @@ impl Tool for RoutineUpdateTool { // Refresh event cache in case trigger changed self.engine.refresh_event_cache().await; + let verification = verification_result_payload(&routine, verification_reset); let result = serde_json::json!({ - "name": routine.name, + "name": routine.name.clone(), "enabled": routine.enabled, "trigger_type": routine.trigger.type_tag(), "next_fire_at": routine.next_fire_at.map(|t| t.to_rfc3339()), "status": "updated", + "verification": verification, }); Ok(ToolOutput::success(result, start.elapsed())) diff --git a/src/tools/mcp/auth.rs b/src/tools/mcp/auth.rs index 9e8792ad1cc..a16c59554ca 100644 --- a/src/tools/mcp/auth.rs +++ b/src/tools/mcp/auth.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use std::net::IpAddr; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use std::time::Duration; use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; @@ -38,6 +38,38 @@ fn oauth_http_client() -> Result<&'static reqwest::Client, AuthError> { .map_err(Clone::clone) } +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +struct RefreshLockKey { + server_name: String, + user_id: String, +} + +fn refresh_lock_key(server_name: &str, user_id: &str) -> RefreshLockKey { + RefreshLockKey { + server_name: server_name.to_string(), + user_id: user_id.to_string(), + } +} + +async fn refresh_lock(server_name: &str, user_id: &str) -> Arc> { + static LOCKS: std::sync::OnceLock< + tokio::sync::Mutex>>>, + > = std::sync::OnceLock::new(); + + let registry = LOCKS.get_or_init(|| tokio::sync::Mutex::new(HashMap::new())); + let mut locks = registry.lock().await; + locks.retain(|_, lock| lock.strong_count() > 0); + + let key = refresh_lock_key(server_name, user_id); + if let Some(lock) = locks.get(&key).and_then(Weak::upgrade) { + return lock; + } + + let lock = Arc::new(tokio::sync::Mutex::new(())); + locks.insert(key, Arc::downgrade(&lock)); + lock +} + /// Log a debug message when a discovery/auth response is a redirect. /// Helps users diagnose configuration issues when legitimate servers /// redirect and our no-redirect policy causes a failure. @@ -201,6 +233,12 @@ pub struct AccessToken { pub scope: Option, } +#[derive(Debug, Clone)] +struct ClientCredentials { + client_id: String, + client_secret: Option, +} + /// Token response from the authorization server. #[derive(Debug, Deserialize)] struct TokenResponse { @@ -706,40 +744,52 @@ pub async fn authorize_mcp_server( } // Determine client_id and endpoints - let (client_id, authorization_url, token_url, use_pkce, scopes, mut extra_params) = - if let Some(oauth) = &server_config.oauth { - // Pre-configured OAuth - let (auth_url, tok_url) = discover_oauth_endpoints(server_config).await?; - ( - oauth.client_id.clone(), - auth_url, - tok_url, - oauth.use_pkce, - oauth.scopes.clone(), - oauth.extra_params.clone(), - ) - } else { - // Try Dynamic Client Registration - println!(" Discovering OAuth endpoints..."); - let auth_meta = discover_full_oauth_metadata(&server_config.url).await?; - - let registration_endpoint = auth_meta - .registration_endpoint - .ok_or(AuthError::NotSupported)?; - - println!(" Registering client dynamically..."); - let registration = register_client(®istration_endpoint, &redirect_uri).await?; - println!(" Client registered: {}", registration.client_id); + let ( + client_id, + client_secret, + client_secret_expires_at, + authorization_url, + token_url, + use_pkce, + scopes, + mut extra_params, + ) = if let Some(oauth) = &server_config.oauth { + // Pre-configured OAuth + let (auth_url, tok_url) = discover_oauth_endpoints(server_config).await?; + ( + oauth.client_id.clone(), + None, + None, + auth_url, + tok_url, + oauth.use_pkce, + oauth.scopes.clone(), + oauth.extra_params.clone(), + ) + } else { + // Try Dynamic Client Registration + println!(" Discovering OAuth endpoints..."); + let auth_meta = discover_full_oauth_metadata(&server_config.url).await?; - ( - registration.client_id, - auth_meta.authorization_endpoint, - auth_meta.token_endpoint, - true, // Always use PKCE for DCR clients - auth_meta.scopes_supported, - HashMap::new(), - ) - }; + let registration_endpoint = auth_meta + .registration_endpoint + .ok_or(AuthError::NotSupported)?; + + println!(" Registering client dynamically..."); + let registration = register_client(®istration_endpoint, &redirect_uri).await?; + println!(" Client registered: {}", registration.client_id); + + ( + registration.client_id, + registration.client_secret, + registration.client_secret_expires_at, + auth_meta.authorization_endpoint, + auth_meta.token_endpoint, + true, // Always use PKCE for DCR clients + auth_meta.scopes_supported, + HashMap::new(), + ) + }; // Generate PKCE challenge let pkce = if use_pkce { @@ -797,6 +847,7 @@ pub async fn authorize_mcp_server( let token = exchange_code_for_token( &token_url, &client_id, + client_secret.as_deref(), &code, &redirect_uri, pkce.as_ref(), @@ -810,6 +861,16 @@ pub async fn authorize_mcp_server( // Store the client_id for DCR (needed for token refresh) if server_config.oauth.is_none() { store_client_id(secrets, user_id, server_config, &client_id).await?; + if let Some(ref client_secret) = client_secret { + store_client_secret( + secrets, + user_id, + server_config, + client_secret, + client_secret_expires_at, + ) + .await?; + } } Ok(token) @@ -893,6 +954,7 @@ pub async fn wait_for_authorization_callback( pub async fn exchange_code_for_token( token_url: &str, client_id: &str, + client_secret: Option<&str>, code: &str, redirect_uri: &str, pkce: Option<&PkceChallenge>, @@ -909,6 +971,10 @@ pub async fn exchange_code_for_token( ("client_id", client_id.to_string()), ]; + if let Some(secret) = client_secret { + params.push(("client_secret", secret.to_string())); + } + if let Some(pkce) = pkce { params.push(("code_verifier", pkce.verifier.clone())); } @@ -1001,6 +1067,31 @@ pub async fn store_client_id( .map_err(|e| AuthError::Secrets(e.to_string())) } +/// Store the DCR client secret for future token refresh. +pub async fn store_client_secret( + secrets: &Arc, + user_id: &str, + server_config: &McpServerConfig, + client_secret: &str, + client_secret_expires_at: Option, +) -> Result<(), AuthError> { + let mut params = + CreateSecretParams::new(server_config.client_secret_secret_name(), client_secret) + .with_provider(format!("mcp:{}", server_config.name)); + + if let Some(expires_at) = client_secret_expires_at + && let Some(dt) = chrono::DateTime::::from_timestamp(expires_at as i64, 0) + { + params = params.with_expiry(dt); + } + + secrets + .create(user_id, params) + .await + .map(|_| ()) + .map_err(|e| AuthError::Secrets(e.to_string())) +} + /// Get the client ID for a server (from config or stored DCR). async fn get_client_id( server_config: &McpServerConfig, @@ -1025,6 +1116,30 @@ async fn get_client_id( } } +/// Get client credentials for a server (from config or stored DCR registration). +async fn get_client_credentials( + server_config: &McpServerConfig, + secrets: &Arc, + user_id: &str, +) -> Result { + let client_id = get_client_id(server_config, secrets, user_id).await?; + let client_secret = match secrets + .get_decrypted(user_id, &server_config.client_secret_secret_name()) + .await + { + Ok(secret) => Some(secret.expose().to_string()), + Err(crate::secrets::SecretError::NotFound(_) | crate::secrets::SecretError::Expired) => { + None + } + Err(e) => return Err(AuthError::Secrets(e.to_string())), + }; + + Ok(ClientCredentials { + client_id, + client_secret, + }) +} + /// Get the stored access token for an MCP server. pub async fn get_access_token( server_config: &McpServerConfig, @@ -1067,8 +1182,28 @@ pub async fn refresh_access_token( secrets: &Arc, user_id: &str, ) -> Result { + let lock = refresh_lock(&server_config.name, user_id).await; + let _guard = lock.lock().await; + + match secrets + .get_decrypted(user_id, &server_config.token_secret_name()) + .await + { + Ok(token) => { + return Ok(AccessToken { + access_token: token.expose().to_string(), + token_type: "Bearer".to_string(), + expires_in: None, + refresh_token: None, + scope: None, + }); + } + Err(crate::secrets::SecretError::Expired | crate::secrets::SecretError::NotFound(_)) => {} + Err(e) => return Err(AuthError::Secrets(e.to_string())), + } + // Get client_id (from config or stored DCR) - let client_id = get_client_id(server_config, secrets, user_id).await?; + let credentials = get_client_credentials(server_config, secrets, user_id).await?; // Get the refresh token (try current name, fall back to legacy name for // users who authenticated before the naming convention was fixed). @@ -1108,45 +1243,82 @@ pub async fn refresh_access_token( validate_url_safe(&token_url).await?; - let client = oauth_http_client()?; - - // Compute canonical resource URI for RFC 8707 - let resource = canonical_resource_uri(&server_config.url); + let token = if let Some(proxy_url) = oauth_defaults::exchange_proxy_url() { + let resource = canonical_resource_uri(&server_config.url); + let provider = format!("mcp:{}", server_config.name); + let gateway_token = oauth_defaults::oauth_proxy_auth_token().ok_or_else(|| { + AuthError::RefreshFailed( + "OAuth refresh proxy is configured but no proxy auth token is available" + .to_string(), + ) + })?; + let token_response = + oauth_defaults::refresh_token_via_proxy(oauth_defaults::ProxyRefreshTokenRequest { + proxy_url: &proxy_url, + gateway_token: &gateway_token, + token_url: &token_url, + client_id: &credentials.client_id, + client_secret: credentials.client_secret.as_deref(), + refresh_token: refresh_token.expose(), + resource: Some(&resource), + provider: Some(provider.as_str()), + }) + .await + .map_err(|e| AuthError::RefreshFailed(e.to_string()))?; + + AccessToken { + access_token: token_response.access_token, + token_type: token_response + .token_type + .unwrap_or_else(|| "Bearer".to_string()), + expires_in: token_response.expires_in, + refresh_token: token_response.refresh_token, + scope: token_response.scope, + } + } else { + let client = oauth_http_client()?; - let params = vec![ - ("grant_type", "refresh_token".to_string()), - ("refresh_token", refresh_token.expose().to_string()), - ("client_id", client_id), - ("resource", resource), - ]; + // Compute canonical resource URI for RFC 8707 + let resource = canonical_resource_uri(&server_config.url); - let response = client - .post(&token_url) - .form(¶ms) - .send() - .await - .map_err(|e| AuthError::RefreshFailed(e.to_string()))?; + let mut params = vec![ + ("grant_type", "refresh_token".to_string()), + ("refresh_token", refresh_token.expose().to_string()), + ("client_id", credentials.client_id.clone()), + ("resource", resource), + ]; + if let Some(client_secret) = credentials.client_secret.as_deref() { + params.push(("client_secret", client_secret.to_string())); + } - if !response.status().is_success() { - let status = response.status(); - let body = response.text().await.unwrap_or_default(); - return Err(AuthError::RefreshFailed(format!( - "HTTP {} - {}", - status, body - ))); - } + let response = client + .post(&token_url) + .form(¶ms) + .send() + .await + .map_err(|e| AuthError::RefreshFailed(e.to_string()))?; - let token_response: TokenResponse = response - .json() - .await - .map_err(|e| AuthError::RefreshFailed(format!("Invalid response: {}", e)))?; + if !response.status().is_success() { + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + return Err(AuthError::RefreshFailed(format!( + "HTTP {} - {}", + status, body + ))); + } - let token = AccessToken { - access_token: token_response.access_token, - token_type: token_response.token_type, - expires_in: token_response.expires_in, - refresh_token: token_response.refresh_token, - scope: token_response.scope, + let token_response: TokenResponse = response + .json() + .await + .map_err(|e| AuthError::RefreshFailed(format!("Invalid response: {}", e)))?; + + AccessToken { + access_token: token_response.access_token, + token_type: token_response.token_type, + expires_in: token_response.expires_in, + refresh_token: token_response.refresh_token, + scope: token_response.scope, + } }; // Store the new tokens @@ -1158,6 +1330,85 @@ pub async fn refresh_access_token( #[cfg(test)] mod tests { use super::*; + use std::sync::Arc; + + use axum::{ + Router, + extract::{Form, State}, + routing::post, + }; + use secrecy::SecretString; + use tokio::net::TcpListener; + use tokio::sync::Mutex; + + use crate::config::helpers::lock_env; + use crate::secrets::{InMemorySecretsStore, SecretsCrypto}; + use crate::testing::credentials::TEST_GATEWAY_CRYPTO_KEY; + + #[derive(Clone, Debug, Default)] + struct RecordedRefreshRequest { + authorization: Option, + form: HashMap, + } + + #[derive(Clone, Default)] + struct MockRefreshState { + requests: Arc>>, + } + + impl MockRefreshState { + async fn requests(&self) -> Vec { + self.requests.lock().await.clone() + } + } + + fn test_secrets_store() -> Arc { + Arc::new(InMemorySecretsStore::new(Arc::new( + SecretsCrypto::new(SecretString::from(TEST_GATEWAY_CRYPTO_KEY.to_string())) + .expect("test crypto"), + ))) + } + + async fn start_refresh_server() -> Option<(String, MockRefreshState)> { + async fn token_handler( + State(state): State, + headers: axum::http::HeaderMap, + Form(form): Form>, + ) -> axum::Json { + state.requests.lock().await.push(RecordedRefreshRequest { + authorization: headers + .get("authorization") + .and_then(|v| v.to_str().ok()) + .map(str::to_string), + form, + }); + axum::Json(serde_json::json!({ + "access_token": "refreshed-access-token", + "token_type": "Bearer", + "refresh_token": "rotated-refresh-token", + "expires_in": 3600 + })) + } + + let state = MockRefreshState::default(); + let app = Router::new() + .route("/token", post(token_handler)) + .route("/oauth/refresh", post(token_handler)) + .with_state(state.clone()); + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(error) if error.kind() == std::io::ErrorKind::PermissionDenied => { + eprintln!("Skipping refresh server test: loopback bind denied by sandbox"); + return None; + } + Err(error) => panic!("failed to bind refresh test server: {error}"), + }; + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + Some((format!("http://127.0.0.1:{}", addr.port()), state)) + } #[test] fn test_pkce_challenge_generation() { @@ -1847,4 +2098,216 @@ mod tests { let state_2 = URL_SAFE_NO_PAD.encode(state_bytes_2); assert_ne!(state, state_2, "State must be unique per request"); } + + #[tokio::test] + async fn test_refresh_access_token_direct_includes_stored_client_secret() { + let secrets = test_secrets_store(); + let user_id = "test-user"; + let Some((base_url, state)) = start_refresh_server().await else { + return; + }; + let server = McpServerConfig::new("notion", "https://mcp.notion.com/mcp").with_oauth( + crate::tools::mcp::config::OAuthConfig::new("configured-client") + .with_endpoints("http://127.0.0.1/authorize", format!("{base_url}/token")), + ); + + secrets + .create( + user_id, + CreateSecretParams::new(server.refresh_token_secret_name(), "refresh-token-123"), + ) + .await + .unwrap(); + store_client_secret(&secrets, user_id, &server, "stored-client-secret", None) + .await + .unwrap(); + + let token = refresh_access_token(&server, &secrets, user_id) + .await + .expect("refresh succeeds"); + assert_eq!(token.access_token, "refreshed-access-token"); + assert_eq!( + token.refresh_token.as_deref(), + Some("rotated-refresh-token") + ); + + let requests = state.requests().await; + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].form.get("client_id").map(String::as_str), + Some("configured-client") + ); + assert_eq!( + requests[0].form.get("client_secret").map(String::as_str), + Some("stored-client-secret") + ); + assert_eq!( + requests[0].form.get("refresh_token").map(String::as_str), + Some("refresh-token-123") + ); + assert_eq!( + requests[0].form.get("resource").map(String::as_str), + Some("https://mcp.notion.com/mcp") + ); + + let stored_refresh = secrets + .get_decrypted(user_id, &server.refresh_token_secret_name()) + .await + .unwrap(); + assert_eq!(stored_refresh.expose(), "rotated-refresh-token"); + } + + #[allow(clippy::await_holding_lock)] + #[tokio::test] + async fn test_refresh_access_token_uses_proxy_when_configured() { + let _env_guard = lock_env(); + let Some((base_url, state)) = start_refresh_server().await else { + return; + }; + let _proxy_url_guard = set_env_var("IRONCLAW_OAUTH_EXCHANGE_URL", Some(&base_url)); + let _proxy_token_guard = set_env_var( + "IRONCLAW_OAUTH_PROXY_AUTH_TOKEN", + Some("gateway-test-token"), + ); + let expected_token_url = format!("{base_url}/token"); + + let secrets = test_secrets_store(); + let user_id = "test-user"; + let server = McpServerConfig::new("notion", "https://mcp.notion.com/mcp").with_oauth( + crate::tools::mcp::config::OAuthConfig::new("configured-client") + .with_endpoints("http://127.0.0.1/authorize", expected_token_url.clone()), + ); + + secrets + .create( + user_id, + CreateSecretParams::new(server.refresh_token_secret_name(), "refresh-token-123"), + ) + .await + .unwrap(); + store_client_secret(&secrets, user_id, &server, "stored-client-secret", None) + .await + .unwrap(); + + refresh_access_token(&server, &secrets, user_id) + .await + .expect("proxy refresh succeeds"); + + let requests = state.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(expected_token_url.as_str()) + ); + assert_eq!( + requests[0].form.get("provider").map(String::as_str), + Some("mcp:notion") + ); + assert_eq!( + requests[0].form.get("client_secret").map(String::as_str), + Some("stored-client-secret") + ); + assert_eq!( + requests[0].form.get("resource").map(String::as_str), + Some("https://mcp.notion.com/mcp") + ); + } + + #[allow(clippy::await_holding_lock)] + #[tokio::test] + async fn test_refresh_access_token_serializes_concurrent_refreshes() { + let _env_guard = lock_env(); + let _proxy_url_guard = set_env_var("IRONCLAW_OAUTH_EXCHANGE_URL", None); + let _proxy_token_guard = set_env_var("IRONCLAW_OAUTH_PROXY_AUTH_TOKEN", None); + let secrets = test_secrets_store(); + let user_id = "test-user"; + let Some((base_url, state)) = start_refresh_server().await else { + return; + }; + let server = McpServerConfig::new("notion", "https://mcp.notion.com/mcp").with_oauth( + crate::tools::mcp::config::OAuthConfig::new("configured-client") + .with_endpoints("http://127.0.0.1/authorize", format!("{base_url}/token")), + ); + + secrets + .create( + user_id, + CreateSecretParams::new(server.refresh_token_secret_name(), "refresh-token-123"), + ) + .await + .unwrap(); + + let (first, second) = tokio::join!( + refresh_access_token(&server, &secrets, user_id), + refresh_access_token(&server, &secrets, user_id), + ); + assert!(first.is_ok(), "first refresh should succeed: {first:?}"); + assert!(second.is_ok(), "second refresh should succeed: {second:?}"); + + let requests = state.requests().await; + assert_eq!( + requests.len(), + 1, + "only one outbound refresh should run for concurrent callers" + ); + } + + #[tokio::test] + async fn test_refresh_lock_reuses_same_key() { + let first = refresh_lock("notion", "user-a").await; + let second = refresh_lock("notion", "user-a").await; + let other_user = refresh_lock("notion", "user-b").await; + + assert!(Arc::ptr_eq(&first, &second)); + assert!(!Arc::ptr_eq(&first, &other_user)); + } + + #[tokio::test] + async fn test_refresh_lock_recreates_dropped_entry() { + let first = refresh_lock("notion-recreate", "user-recreate").await; + let first_weak = Arc::downgrade(&first); + drop(first); + + assert!(first_weak.upgrade().is_none()); + + let second = refresh_lock("notion-recreate", "user-recreate").await; + let third = refresh_lock("notion-recreate", "user-recreate").await; + + assert!(Arc::ptr_eq(&second, &third)); + } + + struct EnvVarGuard { + key: &'static str, + original: 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.original { + std::env::set_var(self.key, value); + } else { + std::env::remove_var(self.key); + } + } + } + } + + fn set_env_var(key: &'static str, value: Option<&str>) -> EnvVarGuard { + let original = std::env::var(key).ok(); + // SAFETY: Tests use lock_env() to serialize environment access. + unsafe { + if let Some(value) = value { + std::env::set_var(key, value); + } else { + std::env::remove_var(key); + } + } + EnvVarGuard { key, original } + } } diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index c7eb62f649c..28e9df515a1 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -270,6 +270,11 @@ impl McpServerConfig { pub fn client_id_secret_name(&self) -> String { format!("mcp_{}_client_id", self.name) } + + /// Get the secret name used to store the DCR client secret. + pub fn client_secret_secret_name(&self) -> String { + format!("mcp_{}_client_secret", self.name) + } } /// OAuth 2.1 configuration for an MCP server. @@ -773,6 +778,11 @@ mod tests { config.legacy_refresh_token_secret_name(), "mcp_notion_refresh_token" ); + assert_eq!(config.client_id_secret_name(), "mcp_notion_client_id"); + assert_eq!( + config.client_secret_secret_name(), + "mcp_notion_client_secret" + ); } #[test] diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index ee0c047c4d9..f8f69ba435a 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -1266,6 +1266,7 @@ async fn refresh_oauth_token( client_id: &config.client_id, client_secret: config.client_secret.as_deref(), refresh_token: refresh_secret.expose(), + resource: None, provider: config.provider.as_deref(), }, ) @@ -1364,6 +1365,14 @@ async fn refresh_oauth_token( .and_then(|v| v.as_str()) .map(str::to_string), expires_in: token_data.get("expires_in").and_then(|v| v.as_u64()), + token_type: token_data + .get("token_type") + .and_then(|v| v.as_str()) + .map(str::to_string), + scope: token_data + .get("scope") + .and_then(|v| v.as_str()) + .map(str::to_string), }, None => { tracing::warn!("Token refresh response missing access_token field"); diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py index f049e5049e3..4119a1ab486 100644 --- a/tests/e2e/mock_llm.py +++ b/tests/e2e/mock_llm.py @@ -43,6 +43,11 @@ "max_results": 1, }, ), + ( + re.compile(r"check mock mcp|mock mcp search", re.IGNORECASE), + "mock-mcp_mock_search", + lambda _: {"query": "refresh-check"}, + ), (re.compile(r"what time|current time", re.IGNORECASE), "time", lambda _: {"operation": "now"}), ( re.compile( @@ -408,15 +413,29 @@ async def oauth_refresh(request: web.Request) -> web.Response: 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) + + provider = data.get("provider", "") + if provider.startswith("mcp:"): + if data.get("client_id") != "mock-mcp-client-id": + return web.json_response({"error": "invalid_mcp_client_id"}, status=400) + if data.get("client_secret") != "mock-mcp-client-secret": + return web.json_response({"error": "missing_mcp_client_secret"}, status=400) + if not data.get("token_url", "").endswith("/oauth/token"): + return web.json_response({"error": "invalid_mcp_token_url"}, status=400) + if data.get("resource") != f"http://127.0.0.1:{request.app['port']}/mcp": + return web.json_response({"error": "missing_mcp_resource"}, status=400) + else: + 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", + "token_type": "Bearer", "refresh_token": "mock-rotated-refresh-token", "expires_in": 3600, + "scope": "mock-scope", }) @@ -542,6 +561,7 @@ async def mcp_oauth_register(request: web.Request) -> web.Response: body = await request.json() return web.json_response({ "client_id": "mock-mcp-client-id", + "client_secret": "mock-mcp-client-secret", "client_name": body.get("client_name", "IronClaw"), "redirect_uris": body.get("redirect_uris", []), }) diff --git a/tests/e2e/scenarios/test_oauth_refresh.py b/tests/e2e/scenarios/test_oauth_refresh.py index 50871f7f946..cd38f88fa14 100644 --- a/tests/e2e/scenarios/test_oauth_refresh.py +++ b/tests/e2e/scenarios/test_oauth_refresh.py @@ -121,6 +121,37 @@ async def _wait_for_gmail_tool_call(base_url: str, thread_id: str, timeout: floa raise AssertionError(f"Timed out waiting for gmail tool call in thread {thread_id}") +async def _wait_for_tool_call( + base_url: str, + thread_id: str, + tool_name: 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") == tool_name: + return history + + await asyncio.sleep(0.5) + + raise AssertionError(f"Timed out waiting for {tool_name} 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) @@ -225,3 +256,107 @@ async def test_hosted_gmail_oauth_refresh_uses_proxy(hosted_oauth_refresh_server assert updated_before_dt is not None assert expires_after_dt > datetime.now(timezone.utc) assert updated_after_dt > updated_before_dt + + +async def test_hosted_mcp_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"] + mcp_url = f"{mock_base_url}/mcp" + + install_response = await api_post( + server, + "/api/extensions/install", + json={"name": "mock-mcp", "url": mcp_url, "kind": "mcp_server"}, + timeout=30, + ) + assert install_response.status_code == 200, install_response.text + assert install_response.json().get("success") is True, install_response.text + + activate_response = await api_post( + server, + "/api/extensions/mock-mcp/activate", + timeout=30, + ) + assert activate_response.status_code == 200, activate_response.text + auth_url = activate_response.json().get("auth_url") + assert auth_url, activate_response.json() + + async with httpx.AsyncClient() as client: + callback_response = await client.get( + f"{server}/oauth/callback", + params={"code": "mock_mcp_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 + + mock_mcp = await _get_extension(server, "mock-mcp") + assert mock_mcp is not None, "mock-mcp should be installed" + assert mock_mcp["authenticated"] is True, mock_mcp + assert "mock-mcp_mock_search" in mock_mcp.get("tools", []), mock_mcp + + client_secret_user_id, _, _ = _find_secret_row(db_path, "mcp_mock-mcp_client_secret") + stored_user_id, expires_before, updated_before = _find_secret_row( + db_path, "mcp_mock-mcp_access_token" + ) + assert client_secret_user_id == stored_user_id + assert _parse_timestamp(expires_before) is not None + assert _parse_timestamp(updated_before) is not None + + await _reset_mock_oauth_state(mock_base_url) + await asyncio.sleep(0.1) + _expire_access_token(db_path, stored_user_id, "mcp_mock-mcp_access_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 mock mcp", "thread_id": thread_id}, + timeout=30, + ) + assert send_response.status_code == 202, send_response.text + + history = await _wait_for_tool_call(server, thread_id, "mock-mcp_mock_search") + assert any( + tool_call.get("name") == "mock-mcp_mock_search" + 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"]["provider"] == "mcp:mock-mcp" + assert last_refresh["form"]["client_id"] == "mock-mcp-client-id" + assert last_refresh["form"]["client_secret"] == "mock-mcp-client-secret" + assert last_refresh["form"]["token_url"].endswith("/oauth/token") + assert last_refresh["form"]["resource"] == mcp_url + + refreshed_user_id, expires_after, updated_after = _find_secret_row( + db_path, "mcp_mock-mcp_access_token" + ) + refresh_user_id, _, refresh_updated_after = _find_secret_row( + db_path, "mcp_mock-mcp_access_token_refresh_token" + ) + assert refreshed_user_id == stored_user_id + assert refresh_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) + refresh_updated_after_dt = _parse_timestamp(refresh_updated_after) + assert expires_after_dt is not None + assert updated_after_dt is not None + assert updated_before_dt is not None + assert refresh_updated_after_dt is not None + assert expires_after_dt > datetime.now(timezone.utc) + assert updated_after_dt > updated_before_dt + assert refresh_updated_after_dt >= updated_after_dt diff --git a/tests/fixtures/llm_traces/advanced/routine_event_any_channel.json b/tests/fixtures/llm_traces/advanced/routine_event_any_channel.json index 61edf1e2a11..d83d9fbfa12 100644 --- a/tests/fixtures/llm_traces/advanced/routine_event_any_channel.json +++ b/tests/fixtures/llm_traces/advanced/routine_event_any_channel.json @@ -29,7 +29,7 @@ { "response": { "type": "text", - "content": "Created the any-channel-bug-watcher routine for bug messages.", + "content": "Created the any-channel-bug-watcher routine for bug messages, but it is not verified yet. It should stay unverified until it has a successful run.", "input_tokens": 170, "output_tokens": 18 } diff --git a/tests/fixtures/llm_traces/advanced/routine_event_telegram.json b/tests/fixtures/llm_traces/advanced/routine_event_telegram.json index b17c5382875..39ebc8054f4 100644 --- a/tests/fixtures/llm_traces/advanced/routine_event_telegram.json +++ b/tests/fixtures/llm_traces/advanced/routine_event_telegram.json @@ -30,7 +30,7 @@ { "response": { "type": "text", - "content": "Created the telegram-bug-watcher routine for Telegram bug messages.", + "content": "Created the telegram-bug-watcher routine for Telegram bug messages, but it is not verified yet. I can test it the next time you want to fire it.", "input_tokens": 180, "output_tokens": 20 } diff --git a/tests/fixtures/llm_traces/advanced/routine_news_digest.json b/tests/fixtures/llm_traces/advanced/routine_news_digest.json index 4c98b49f656..5b7e1dfd111 100644 --- a/tests/fixtures/llm_traces/advanced/routine_news_digest.json +++ b/tests/fixtures/llm_traces/advanced/routine_news_digest.json @@ -37,7 +37,7 @@ { "response": { "type": "text", - "content": "Created the **morning-tech-news** routine with manual trigger and full_job mode. The `message` and `http` tools are pre-authorized.", + "content": "Created the **morning-tech-news** routine with manual trigger and full_job mode. The `message` and `http` tools are available, but the routine is not verified yet.", "input_tokens": 200, "output_tokens": 50 } diff --git a/tests/fixtures/llm_traces/tools/routine_create_grouped.json b/tests/fixtures/llm_traces/tools/routine_create_grouped.json index ae4b6eb95af..417c809db07 100644 --- a/tests/fixtures/llm_traces/tools/routine_create_grouped.json +++ b/tests/fixtures/llm_traces/tools/routine_create_grouped.json @@ -57,7 +57,7 @@ { "response": { "type": "text", - "content": "Created the weekday-digest routine with a grouped cron request and listed the active routines.", + "content": "Created the weekday-digest routine with a grouped cron request and listed the routines. It is not verified yet, so it should stay unverified until it has a successful run.", "input_tokens": 250, "output_tokens": 24 } diff --git a/tests/fixtures/llm_traces/tools/routine_create_list.json b/tests/fixtures/llm_traces/tools/routine_create_list.json index 114bae16c32..05bce4a97cb 100644 --- a/tests/fixtures/llm_traces/tools/routine_create_list.json +++ b/tests/fixtures/llm_traces/tools/routine_create_list.json @@ -52,7 +52,7 @@ { "response": { "type": "text", - "content": "I created a daily-check routine that runs at 9 AM every day. The routine list shows it as active.", + "content": "I created the daily-check routine, but it is not verified yet. It is scheduled for 9 AM every day, and the routine list should show it as unverified until it has a successful run.", "input_tokens": 300, "output_tokens": 25 } diff --git a/tests/fixtures/llm_traces/tools/routine_history.json b/tests/fixtures/llm_traces/tools/routine_history.json index 0b397f9bfc0..50e10b075fc 100644 --- a/tests/fixtures/llm_traces/tools/routine_history.json +++ b/tests/fixtures/llm_traces/tools/routine_history.json @@ -41,7 +41,7 @@ { "response": { "type": "text", - "content": "The history-test routine was created. Its run history is empty since it hasn't been triggered yet.", + "content": "The history-test routine was created, but it is not verified yet. Its run history is empty since it hasn't been triggered yet.", "input_tokens": 300, "output_tokens": 25 } diff --git a/tests/fixtures/llm_traces/tools/routine_manual_create.json b/tests/fixtures/llm_traces/tools/routine_manual_create.json index bf3862637c8..c153c5d2178 100644 --- a/tests/fixtures/llm_traces/tools/routine_manual_create.json +++ b/tests/fixtures/llm_traces/tools/routine_manual_create.json @@ -27,7 +27,7 @@ { "response": { "type": "text", - "content": "Created the manual-triage routine. It will only run when explicitly fired.", + "content": "Created the manual-triage routine, but it is not verified yet. It will only run when explicitly fired, so I can test it for you when you're ready.", "input_tokens": 140, "output_tokens": 18 } diff --git a/tests/fixtures/llm_traces/tools/routine_manual_create_no_tools.json b/tests/fixtures/llm_traces/tools/routine_manual_create_no_tools.json index 275f2269034..41f981907e9 100644 --- a/tests/fixtures/llm_traces/tools/routine_manual_create_no_tools.json +++ b/tests/fixtures/llm_traces/tools/routine_manual_create_no_tools.json @@ -30,7 +30,7 @@ { "response": { "type": "text", - "content": "Created the manual-triage-no-tools routine. It will only run when explicitly fired and stay text-only.", + "content": "Created the manual-triage-no-tools routine, but it is not verified yet. It will only run when explicitly fired and stay text-only until you decide to test it.", "input_tokens": 140, "output_tokens": 18 } diff --git a/tests/fixtures/llm_traces/tools/routine_update_delete.json b/tests/fixtures/llm_traces/tools/routine_update_delete.json index 81f364f84ea..8479a3bfa94 100644 --- a/tests/fixtures/llm_traces/tools/routine_update_delete.json +++ b/tests/fixtures/llm_traces/tools/routine_update_delete.json @@ -59,7 +59,7 @@ { "response": { "type": "text", - "content": "Created, updated, and then deleted the temp-routine successfully.", + "content": "Created, updated, and then deleted the temp-routine successfully. The update would have left it unverified until it was tested again.", "input_tokens": 400, "output_tokens": 20 } diff --git a/tests/gateway_workflow_integration.rs b/tests/gateway_workflow_integration.rs index 1cd74acb842..3842c1ea1b5 100644 --- a/tests/gateway_workflow_integration.rs +++ b/tests/gateway_workflow_integration.rs @@ -16,6 +16,7 @@ mod tests { use chrono::Utc; use ironclaw::agent::routine::{ NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, + reset_routine_verification_state, routine_verification_fingerprint, }; use uuid::Uuid; @@ -339,6 +340,117 @@ mod tests { mock.shutdown().await; } + #[tokio::test] + async fn routines_api_surfaces_unverified_status_for_new_routine() { + let mock = MockOpenAiServerBuilder::new() + .with_default_response(MockOpenAiResponse::Text("ack".to_string())) + .start() + .await; + + let harness = + GatewayWorkflowHarness::start_openai_compatible(&mock.openai_base_url(), "mock-model") + .await; + + let mut routine = Routine { + id: Uuid::new_v4(), + name: "wf-unverified".to_string(), + description: "Unverified status regression test".to_string(), + user_id: harness.user_id.clone(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::Lightweight { + prompt: "Check verification status".to_string(), + context_paths: Vec::new(), + max_tokens: 512, + use_tools: false, + max_tool_rounds: 1, + }, + guardrails: RoutineGuardrails { + cooldown: Duration::from_secs(0), + max_concurrent: 1, + dedup_window: None, + }, + notify: NotifyConfig::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(), + }; + routine.state = reset_routine_verification_state( + &routine.state, + routine_verification_fingerprint(&routine), + ); + harness + .db + .create_routine(&routine) + .await + .expect("create routine"); + + let mut disabled_routine = routine.clone(); + disabled_routine.id = Uuid::new_v4(); + disabled_routine.name = "wf-unverified-disabled".to_string(); + disabled_routine.enabled = false; + disabled_routine.state = reset_routine_verification_state( + &disabled_routine.state, + routine_verification_fingerprint(&disabled_routine), + ); + harness + .db + .create_routine(&disabled_routine) + .await + .expect("create disabled routine"); + + let list = harness.list_routines().await; + let routine_id = routine.id.to_string(); + let listed = list["routines"] + .as_array() + .expect("routines array") + .iter() + .find(|item| item["id"].as_str() == Some(routine_id.as_str())) + .expect("routine should be listed"); + assert_eq!(listed["status"].as_str(), Some("unverified")); + assert_eq!(listed["verification_status"].as_str(), Some("unverified")); + + let summary = harness + .client + .get(format!("{}/api/routines/summary", harness.base_url())) + .bearer_auth(&harness.auth_token) + .send() + .await + .expect("summary request failed") + .error_for_status() + .expect("summary non-2xx") + .json::() + .await + .expect("invalid summary response"); + assert_eq!(summary["unverified"].as_u64(), Some(2)); + + let detail = harness + .client + .get(format!( + "{}/api/routines/{}", + harness.base_url(), + routine_id + )) + .bearer_auth(&harness.auth_token) + .send() + .await + .expect("detail request failed") + .error_for_status() + .expect("detail non-2xx") + .json::() + .await + .expect("invalid detail response"); + assert_eq!(detail["status"].as_str(), Some("unverified")); + assert_eq!(detail["verification_status"].as_str(), Some("unverified")); + + harness.shutdown().await; + mock.shutdown().await; + } + /// Regression test for issue #1076: web API toggle must immediately /// invalidate the in-memory event cache so disabled routines stop firing. #[tokio::test]