From cac6f4013c3003c901aecc77fc6f32b8ef2718e0 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Thu, 19 Mar 2026 18:32:47 -0700 Subject: [PATCH 1/3] Add owner-scoped permissions for full-job routines (#1440) * docs: add comments explaining CLI_ENABLED=false in service templates (#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes #990 Co-Authored-By: Claude Opus 4.6 (1M context) * Add owner-scoped full-job routine permissions * Address PR review feedback * Fix owner gate test timing --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/agent/routine.rs | 252 ++++++++++++++++- src/agent/routine_engine.rs | 68 +++-- src/channels/web/handlers/routines.rs | 45 ++- src/channels/web/server.rs | 163 +---------- src/channels/web/static/app.js | 44 ++- src/channels/web/static/i18n/en.js | 4 + src/channels/web/static/i18n/zh-CN.js | 4 + src/channels/web/types.rs | 11 + src/service.rs | 2 + src/tools/builtin/routine.rs | 387 ++++++++++++++++++++++++-- tests/dispatched_routine_run_tests.rs | 4 +- tests/e2e_builtin_tool_coverage.rs | 6 +- tests/e2e_routine_heartbeat.rs | 383 ++++++++++++++++++++++++- tests/gateway_workflow_integration.rs | 106 +++++++ 14 files changed, 1247 insertions(+), 232 deletions(-) diff --git a/src/agent/routine.rs b/src/agent/routine.rs index f3850fa0..7d87bd9a 100644 --- a/src/agent/routine.rs +++ b/src/agent/routine.rs @@ -17,7 +17,7 @@ //! └──────────────┘ //! ``` -use std::collections::hash_map::DefaultHasher; +use std::collections::{HashSet, hash_map::DefaultHasher}; use std::hash::{Hash, Hasher}; use std::str::FromStr; use std::time::Duration; @@ -28,6 +28,171 @@ use uuid::Uuid; use crate::error::RoutineError; +pub const FULL_JOB_OWNER_ALLOWED_TOOLS_SETTING_KEY: &str = "routines.full_job_owner_allowed_tools"; +pub const FULL_JOB_DEFAULT_PERMISSION_MODE_SETTING_KEY: &str = + "routines.full_job_default_permission_mode"; + +/// Persisted per-routine permission mode for autonomous `full_job` routines. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "snake_case")] +pub enum FullJobPermissionMode { + /// Only use the routine's stored `tool_permissions`. + #[default] + Explicit, + /// Union the owner-scoped allowlist with the routine's `tool_permissions`. + InheritOwner, +} + +impl FullJobPermissionMode { + pub fn as_str(self) -> &'static str { + match self { + Self::Explicit => "explicit", + Self::InheritOwner => "inherit_owner", + } + } +} + +impl FromStr for FullJobPermissionMode { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "explicit" => Ok(Self::Explicit), + "inherit_owner" => Ok(Self::InheritOwner), + _ => Err(()), + } + } +} + +/// Owner-scoped default behavior for newly-created `full_job` routines. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum FullJobPermissionDefaultMode { + Explicit, + #[default] + InheritOwner, + CopyOwner, +} + +impl FullJobPermissionDefaultMode { + pub fn as_str(self) -> &'static str { + match self { + Self::Explicit => "explicit", + Self::InheritOwner => "inherit_owner", + Self::CopyOwner => "copy_owner", + } + } +} + +impl FromStr for FullJobPermissionDefaultMode { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "explicit" => Ok(Self::Explicit), + "inherit_owner" => Ok(Self::InheritOwner), + "copy_owner" => Ok(Self::CopyOwner), + _ => Err(()), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct FullJobPermissionSettings { + pub owner_allowed_tools: Vec, + pub default_mode: FullJobPermissionDefaultMode, +} + +pub fn normalize_tool_names(tools: I) -> Vec +where + I: IntoIterator, +{ + let mut seen = HashSet::new(); + let mut normalized = Vec::new(); + for tool in tools { + let trimmed = tool.trim(); + if trimmed.is_empty() { + continue; + } + let normalized_name = trimmed.to_string(); + if seen.insert(normalized_name.clone()) { + normalized.push(normalized_name); + } + } + normalized +} + +pub fn parse_full_job_permission_mode(value: &serde_json::Value) -> FullJobPermissionMode { + value + .get("permission_mode") + .and_then(|v| v.as_str()) + .and_then(|mode| FullJobPermissionMode::from_str(mode).ok()) + .unwrap_or_default() +} + +fn parse_owner_allowed_tools_setting(value: Option) -> Vec { + match value { + Some(serde_json::Value::Array(values)) => normalize_tool_names( + values + .into_iter() + .filter_map(|value| value.as_str().map(ToOwned::to_owned)), + ), + Some(serde_json::Value::String(csv)) => normalize_tool_names( + csv.split([',', '\n']) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(ToOwned::to_owned), + ), + _ => Vec::new(), + } +} + +fn parse_default_permission_mode_setting( + value: Option, +) -> FullJobPermissionDefaultMode { + value + .and_then(|v| v.as_str().map(ToOwned::to_owned)) + .and_then(|mode| FullJobPermissionDefaultMode::from_str(&mode).ok()) + .unwrap_or_default() +} + +pub async fn load_full_job_permission_settings( + store: &(dyn crate::db::SettingsStore + Sync), + user_id: &str, +) -> Result { + let owner_allowed_tools = parse_owner_allowed_tools_setting( + store + .get_setting(user_id, FULL_JOB_OWNER_ALLOWED_TOOLS_SETTING_KEY) + .await?, + ); + let default_mode = parse_default_permission_mode_setting( + store + .get_setting(user_id, FULL_JOB_DEFAULT_PERMISSION_MODE_SETTING_KEY) + .await?, + ); + Ok(FullJobPermissionSettings { + owner_allowed_tools, + default_mode, + }) +} + +pub fn effective_full_job_tool_permissions( + permission_mode: FullJobPermissionMode, + routine_tool_permissions: &[String], + owner_allowed_tools: &[String], +) -> Vec { + match permission_mode { + FullJobPermissionMode::Explicit => { + normalize_tool_names(routine_tool_permissions.iter().cloned()) + } + FullJobPermissionMode::InheritOwner => normalize_tool_names( + owner_allowed_tools + .iter() + .cloned() + .chain(routine_tool_permissions.iter().cloned()), + ), + } +} + /// A routine is a named, persistent, user-owned task with a trigger and an action. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Routine { @@ -240,6 +405,10 @@ pub enum RoutineAction { /// automatically permitted in routine jobs without listing them here. #[serde(default)] tool_permissions: Vec, + /// Whether this routine should inherit the owner's durable full-job + /// permission allowlist or use only its explicit `tool_permissions`. + #[serde(default)] + permission_mode: FullJobPermissionMode, }, } @@ -266,15 +435,14 @@ fn clamp_max_tool_rounds(value: u64) -> u32 { /// Parse a `tool_permissions` JSON array into a `Vec`. pub fn parse_tool_permissions(value: &serde_json::Value) -> Vec { - value - .get("tool_permissions") - .and_then(|v| v.as_array()) - .map(|arr| { - arr.iter() - .filter_map(|v| v.as_str().map(String::from)) - .collect() - }) - .unwrap_or_default() + normalize_tool_names( + value + .get("tool_permissions") + .and_then(|v| v.as_array()) + .into_iter() + .flatten() + .filter_map(|v| v.as_str().map(String::from)), + ) } impl RoutineAction { @@ -352,11 +520,13 @@ impl RoutineAction { .unwrap_or(default_max_iterations() as u64) as u32; let tool_permissions = parse_tool_permissions(&config); + let permission_mode = parse_full_job_permission_mode(&config); Ok(RoutineAction::FullJob { title, description, max_iterations, tool_permissions, + permission_mode, }) } other => Err(RoutineError::UnknownActionType { @@ -386,11 +556,13 @@ impl RoutineAction { description, max_iterations, tool_permissions, + permission_mode, } => serde_json::json!({ "title": title, "description": description, "max_iterations": max_iterations, "tool_permissions": tool_permissions, + "permission_mode": permission_mode, }), } } @@ -704,8 +876,8 @@ 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, + FullJobPermissionMode, MAX_TOOL_ROUNDS_LIMIT, RoutineAction, RoutineGuardrails, RunStatus, + Trigger, content_hash, describe_cron, effective_full_job_tool_permissions, next_cron_fire, }; #[test] @@ -773,15 +945,67 @@ mod tests { description: "Review and deploy pending changes".to_string(), max_iterations: 5, tool_permissions: vec!["shell".to_string()], + permission_mode: FullJobPermissionMode::InheritOwner, }; let json = action.to_config_json(); let parsed = RoutineAction::from_db("full_job", json).expect("parse full_job"); assert!( - matches!(parsed, RoutineAction::FullJob { title, max_iterations, tool_permissions, .. } - if title == "Deploy review" && max_iterations == 5 && tool_permissions == vec!["shell".to_string()]) + matches!(parsed, RoutineAction::FullJob { title, max_iterations, tool_permissions, permission_mode, .. } + if title == "Deploy review" + && max_iterations == 5 + && tool_permissions == vec!["shell".to_string()] + && permission_mode == FullJobPermissionMode::InheritOwner) ); } + #[test] + fn test_action_full_job_missing_permission_mode_defaults_to_explicit() { + let parsed = RoutineAction::from_db( + "full_job", + serde_json::json!({ + "title": "Deploy review", + "description": "Review and deploy pending changes", + "max_iterations": 5, + "tool_permissions": ["shell"] + }), + ) + .expect("parse full_job"); + assert!(matches!( + parsed, + RoutineAction::FullJob { + permission_mode: FullJobPermissionMode::Explicit, + .. + } + )); + } + + #[test] + fn test_effective_full_job_tool_permissions_inherit_owner_unions_lists() { + let resolved = effective_full_job_tool_permissions( + FullJobPermissionMode::InheritOwner, + &["shell".to_string(), "message".to_string()], + &["message".to_string(), "http".to_string()], + ); + assert_eq!( + resolved, + vec![ + "message".to_string(), + "http".to_string(), + "shell".to_string() + ] + ); + } + + #[test] + fn test_effective_full_job_tool_permissions_explicit_ignores_owner_defaults() { + let resolved = effective_full_job_tool_permissions( + FullJobPermissionMode::Explicit, + &["shell".to_string()], + &["message".to_string(), "http".to_string()], + ); + assert_eq!(resolved, vec!["shell".to_string()]); + } + #[test] fn test_run_status_display_parse() { for status in [ diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 2487ac05..6e216fdc 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -22,7 +22,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, + effective_full_job_tool_permissions, load_full_job_permission_settings, next_cron_fire, }; use crate::channels::OutgoingResponse; use crate::config::RoutineConfig; @@ -890,17 +891,16 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) description, max_iterations, tool_permissions, + permission_mode, } => { - execute_full_job( - &ctx, - &routine, - &run, + let execution = FullJobExecutionConfig { title, description, - *max_iterations, + max_iterations: *max_iterations, tool_permissions, - ) - .await + permission_mode: *permission_mode, + }; + execute_full_job(&ctx, &routine, &run, &execution).await } }; @@ -1026,14 +1026,19 @@ fn sanitize_routine_name(name: &str) -> String { /// non-active state (not Pending/InProgress/Stuck). Returns the final /// `RunStatus` mapped from the job outcome. This keeps the routine run /// active for the full job lifetime so concurrency guardrails apply. +struct FullJobExecutionConfig<'a> { + title: &'a str, + description: &'a str, + max_iterations: u32, + tool_permissions: &'a [String], + permission_mode: crate::agent::routine::FullJobPermissionMode, +} + async fn execute_full_job( ctx: &EngineContext, routine: &Routine, run: &RoutineRun, - title: &str, - description: &str, - max_iterations: u32, - tool_permissions: &[String], + execution: &FullJobExecutionConfig<'_>, ) -> Result<(RunStatus, Option, Option), RoutineError> { let scheduler = ctx .scheduler @@ -1042,8 +1047,10 @@ async fn execute_full_job( reason: "scheduler not available".to_string(), })?; - let mut metadata = - serde_json::json!({ "max_iterations": max_iterations, "owner_id": routine.user_id }); + let mut metadata = serde_json::json!({ + "max_iterations": execution.max_iterations, + "owner_id": routine.user_id + }); // Carry the routine's notify config in job metadata so the message tool // can resolve channel/target per-job without global state mutation. if let Some(channel) = &routine.notify.channel { @@ -1051,15 +1058,38 @@ async fn execute_full_job( } metadata["notify_user"] = serde_json::json!(&routine.notify.user); + let effective_permissions = match execution.permission_mode { + crate::agent::routine::FullJobPermissionMode::Explicit => { + effective_full_job_tool_permissions( + execution.permission_mode, + execution.tool_permissions, + &[], + ) + } + crate::agent::routine::FullJobPermissionMode::InheritOwner => { + let owner_permissions = + load_full_job_permission_settings(ctx.store.as_ref(), &routine.user_id) + .await + .map_err(|e| RoutineError::Database { + reason: format!("failed to load routine permission settings: {e}"), + })?; + effective_full_job_tool_permissions( + execution.permission_mode, + execution.tool_permissions, + &owner_permissions.owner_allowed_tools, + ) + } + }; + // Build approval context: UnlessAutoApproved tools are auto-approved for routines; - // Always tools require explicit listing in tool_permissions. - let approval_context = ApprovalContext::autonomous_with_tools(tool_permissions.iter().cloned()); + // Always tools require explicit listing in the resolved effective permissions. + let approval_context = ApprovalContext::autonomous_with_tools(effective_permissions); let job_id = scheduler .dispatch_job_with_context( &routine.user_id, - title, - description, + execution.title, + execution.description, Some(metadata), approval_context, ) @@ -1082,7 +1112,7 @@ async fn execute_full_job( tracing::info!( routine = %routine.name, job_id = %job_id, - max_iterations = max_iterations, + max_iterations = execution.max_iterations, "Dispatched full job for routine, watching for completion" ); diff --git a/src/channels/web/handlers/routines.rs b/src/channels/web/handlers/routines.rs index 41bfee5a..99d31991 100644 --- a/src/channels/web/handlers/routines.rs +++ b/src/channels/web/handlers/routines.rs @@ -10,11 +10,29 @@ use axum::{ use serde::Deserialize; use uuid::Uuid; -use crate::agent::routine::{Trigger, next_cron_fire}; +use crate::agent::routine::{ + FullJobPermissionDefaultMode, FullJobPermissionMode, RoutineAction, Trigger, + effective_full_job_tool_permissions, load_full_job_permission_settings, next_cron_fire, +}; use crate::channels::web::server::GatewayState; use crate::channels::web::types::*; use crate::error::RoutineError; +fn permission_mode_label(mode: FullJobPermissionMode) -> String { + match mode { + FullJobPermissionMode::Explicit => "explicit".to_string(), + FullJobPermissionMode::InheritOwner => "inherit_owner".to_string(), + } +} + +fn default_permission_mode_label(mode: FullJobPermissionDefaultMode) -> String { + match mode { + FullJobPermissionDefaultMode::Explicit => "explicit".to_string(), + FullJobPermissionDefaultMode::InheritOwner => "inherit_owner".to_string(), + FullJobPermissionDefaultMode::CopyOwner => "copy_owner".to_string(), + } +} + pub async fn routines_list_handler( State(state): State>, ) -> Result, (StatusCode, String)> { @@ -113,6 +131,30 @@ pub async fn routines_detail_handler( }) .collect(); let routine_info = RoutineInfo::from_routine(&routine); + let full_job_permissions = match &routine.action { + RoutineAction::FullJob { + tool_permissions, + permission_mode, + .. + } => { + let owner_settings = + load_full_job_permission_settings(store.as_ref(), &routine.user_id) + .await + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + Some(FullJobPermissionInfo { + permission_mode: permission_mode_label(*permission_mode), + default_permission_mode: default_permission_mode_label(owner_settings.default_mode), + stored_tool_permissions: tool_permissions.clone(), + effective_tool_permissions: effective_full_job_tool_permissions( + *permission_mode, + tool_permissions, + &owner_settings.owner_allowed_tools, + ), + owner_allowed_tools: owner_settings.owner_allowed_tools, + }) + } + RoutineAction::Lightweight { .. } => None, + }; Ok(Json(RoutineDetailResponse { id: routine.id, @@ -131,6 +173,7 @@ pub async fn routines_detail_handler( run_count: routine.run_count, consecutive_failures: routine.consecutive_failures, created_at: routine.created_at.to_rfc3339(), + full_job_permissions, recent_runs, })) } diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index ea3341c0..501852d4 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -36,7 +36,10 @@ use crate::channels::web::handlers::jobs::{ jobs_events_handler, jobs_list_handler, jobs_prompt_handler, jobs_restart_handler, jobs_summary_handler, }; -use crate::channels::web::handlers::routines::{routines_delete_handler, routines_toggle_handler}; +use crate::channels::web::handlers::routines::{ + routines_delete_handler, routines_detail_handler, routines_list_handler, + routines_summary_handler, routines_toggle_handler, routines_trigger_handler, +}; use crate::channels::web::handlers::skills::{ skills_install_handler, skills_list_handler, skills_remove_handler, skills_search_handler, }; @@ -2391,164 +2394,6 @@ async fn pairing_approve_handler( } } -// --- Routines handlers --- - -async fn routines_list_handler( - State(state): State>, -) -> Result, (StatusCode, String)> { - let store = state.store.as_ref().ok_or(( - StatusCode::SERVICE_UNAVAILABLE, - "Database not available".to_string(), - ))?; - - let routines = store - .list_all_routines() - .await - .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; - - let items: Vec = routines.iter().map(RoutineInfo::from_routine).collect(); - - Ok(Json(RoutineListResponse { routines: items })) -} - -async fn routines_summary_handler( - State(state): State>, -) -> Result, (StatusCode, String)> { - let store = state.store.as_ref().ok_or(( - StatusCode::SERVICE_UNAVAILABLE, - "Database not available".to_string(), - ))?; - - let routines = store - .list_all_routines() - .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 today_start = chrono::Utc::now() - .date_naive() - .and_hms_opt(0, 0, 0) - .map(|dt| dt.and_utc()); - let runs_today = if let Some(start) = today_start { - routines - .iter() - .filter(|r| r.last_run_at.is_some_and(|ts| ts >= start)) - .count() as u64 - } else { - 0 - }; - - Ok(Json(RoutineSummaryResponse { - total, - enabled, - disabled, - failing, - runs_today, - })) -} - -async fn routines_detail_handler( - State(state): State>, - Path(id): Path, -) -> Result, (StatusCode, String)> { - let store = state.store.as_ref().ok_or(( - StatusCode::SERVICE_UNAVAILABLE, - "Database not available".to_string(), - ))?; - - let routine_id = Uuid::parse_str(&id) - .map_err(|_| (StatusCode::BAD_REQUEST, "Invalid routine ID".to_string()))?; - - let routine = store - .get_routine(routine_id) - .await - .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? - .ok_or((StatusCode::NOT_FOUND, "Routine not found".to_string()))?; - - let runs = store - .list_routine_runs(routine_id, 20) - .await - .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; - - let recent_runs: Vec = runs - .iter() - .map(|run| RoutineRunInfo { - id: run.id, - trigger_type: run.trigger_type.clone(), - started_at: run.started_at.to_rfc3339(), - completed_at: run.completed_at.map(|dt| dt.to_rfc3339()), - status: format!("{:?}", run.status), - result_summary: run.result_summary.clone(), - tokens_used: run.tokens_used, - job_id: run.job_id, - }) - .collect(); - let routine_info = RoutineInfo::from_routine(&routine); - - Ok(Json(RoutineDetailResponse { - id: routine.id, - name: routine.name.clone(), - description: routine.description.clone(), - enabled: routine.enabled, - trigger_type: routine_info.trigger_type, - trigger_raw: routine_info.trigger_raw, - trigger_summary: routine_info.trigger_summary, - trigger: serde_json::to_value(&routine.trigger).unwrap_or_default(), - action: serde_json::to_value(&routine.action).unwrap_or_default(), - guardrails: serde_json::to_value(&routine.guardrails).unwrap_or_default(), - notify: serde_json::to_value(&routine.notify).unwrap_or_default(), - last_run_at: routine.last_run_at.map(|dt| dt.to_rfc3339()), - next_fire_at: routine.next_fire_at.map(|dt| dt.to_rfc3339()), - run_count: routine.run_count, - consecutive_failures: routine.consecutive_failures, - created_at: routine.created_at.to_rfc3339(), - recent_runs, - })) -} - -async fn routines_trigger_handler( - State(state): State>, - Path(id): Path, -) -> Result, (StatusCode, String)> { - let engine = { - let guard = state.routine_engine.read().await; - guard.as_ref().cloned().ok_or(( - StatusCode::SERVICE_UNAVAILABLE, - "Routine engine not available".to_string(), - ))? - }; - - let routine_id = Uuid::parse_str(&id) - .map_err(|_| (StatusCode::BAD_REQUEST, "Invalid routine ID".to_string()))?; - - let run_id = engine - .fire_manual(routine_id, Some(&state.user_id)) - .await - .map_err(|e| { - let status = match &e { - crate::error::RoutineError::NotFound { .. } => StatusCode::NOT_FOUND, - crate::error::RoutineError::NotAuthorized { .. } => StatusCode::FORBIDDEN, - crate::error::RoutineError::Disabled { .. } - | crate::error::RoutineError::MaxConcurrent { .. } => StatusCode::CONFLICT, - _ => StatusCode::INTERNAL_SERVER_ERROR, - }; - (status, e.to_string()) - })?; - - Ok(Json(serde_json::json!({ - "status": "triggered", - "routine_id": routine_id, - "run_id": run_id, - }))) -} - async fn routines_runs_handler( State(state): State>, Path(id): Path, diff --git a/src/channels/web/static/app.js b/src/channels/web/static/app.js index bc23d68c..8b029068 100644 --- a/src/channels/web/static/app.js +++ b/src/channels/web/static/app.js @@ -3855,6 +3855,17 @@ function renderRoutineDetail(routine) { } // Action config + if (routine.full_job_permissions) { + html += '

Full Job Permissions

' + + '
' + + metaItem('Mode', routine.full_job_permissions.permission_mode) + + metaItem('Owner Default', routine.full_job_permissions.default_permission_mode) + + metaItem('Inherited Tools', (routine.full_job_permissions.owner_allowed_tools || []).join(', ') || '-') + + metaItem('Stored Tools', (routine.full_job_permissions.stored_tool_permissions || []).join(', ') || '-') + + metaItem('Effective Tools', (routine.full_job_permissions.effective_tool_permissions || []).join(', ') || '-') + + '
'; + } + html += '

Action

' + '
' + escapeHtml(JSON.stringify(routine.action, null, 2)) + '
'; @@ -4689,6 +4700,10 @@ var AGENT_SETTINGS = [ settings: [ { key: 'routines.max_concurrent', label: 'cfg.routines_max_concurrent.label', description: 'cfg.routines_max_concurrent.desc', type: 'number', min: 0 }, { key: 'routines.default_cooldown_secs', label: 'cfg.routines_cooldown.label', description: 'cfg.routines_cooldown.desc', type: 'number', min: 0 }, + { key: 'routines.full_job_default_permission_mode', label: 'cfg.routines_full_job_default_mode.label', description: 'cfg.routines_full_job_default_mode.desc', + type: 'select', options: ['inherit_owner', 'explicit', 'copy_owner'] }, + { key: 'routines.full_job_owner_allowed_tools', label: 'cfg.routines_full_job_owner_tools.label', description: 'cfg.routines_full_job_owner_tools.desc', + type: 'list', placeholder: 'shell, http' }, ] }, { @@ -4873,7 +4888,14 @@ function renderStructuredSettingsRow(def, value, activeValue) { inputWrap.style.gap = '8px'; var ariaLabel = I18n.t(def.label) + (def.description ? '. ' + I18n.t(def.description) : ''); - var placeholderText = activeValue ? I18n.t('settings.envValue', { value: activeValue }) : (def.placeholder || I18n.t('settings.envDefault')); + function formatSettingValue(raw) { + if (Array.isArray(raw)) return raw.join(', '); + if (raw === null || raw === undefined) return ''; + return String(raw); + } + + var activeValueText = formatSettingValue(activeValue); + var placeholderText = activeValueText ? I18n.t('settings.envValue', { value: activeValueText }) : (def.placeholder || I18n.t('settings.envDefault')); if (def.type === 'boolean') { var boolSel = document.createElement('select'); @@ -4945,6 +4967,26 @@ function renderStructuredSettingsRow(def, value, activeValue) { }; })(def.key, numInp)); inputWrap.appendChild(numInp); + } else if (def.type === 'list') { + var listInp = document.createElement('input'); + listInp.type = 'text'; + listInp.className = 'settings-input'; + listInp.setAttribute('aria-label', ariaLabel); + var listValue = ''; + if (Array.isArray(value)) listValue = value.join(', '); + else if (typeof value === 'string') listValue = value; + listInp.value = listValue; + if (!listValue) listInp.placeholder = placeholderText; + listInp.addEventListener('change', (function(k, el) { + return function() { + if (el.value.trim() === '') return saveSetting(k, null); + var items = el.value.split(/[\n,]/).map(function(item) { + return item.trim(); + }).filter(Boolean); + saveSetting(k, items); + }; + })(def.key, listInp)); + inputWrap.appendChild(listInp); } else { var textInp = document.createElement('input'); textInp.type = 'text'; diff --git a/src/channels/web/static/i18n/en.js b/src/channels/web/static/i18n/en.js index 1369b485..cd57a400 100644 --- a/src/channels/web/static/i18n/en.js +++ b/src/channels/web/static/i18n/en.js @@ -475,6 +475,10 @@ I18n.register('en', { 'cfg.routines_max_concurrent.desc': 'Maximum routines running simultaneously', 'cfg.routines_cooldown.label': 'Default Cooldown', 'cfg.routines_cooldown.desc': 'Minimum seconds between routine fires', + 'cfg.routines_full_job_default_mode.label': 'Full Job Default Mode', + 'cfg.routines_full_job_default_mode.desc': 'Default permission behavior for new full_job routines. When unset, inherit_owner is used.', + 'cfg.routines_full_job_owner_tools.label': 'Full Job Owner Allowlist', + 'cfg.routines_full_job_owner_tools.desc': 'Comma-separated tool names that full_job routines may inherit at run time.', // Safety settings 'cfg.safety_max_output.label': 'Max Output Length', diff --git a/src/channels/web/static/i18n/zh-CN.js b/src/channels/web/static/i18n/zh-CN.js index 6262b562..028ff5fc 100644 --- a/src/channels/web/static/i18n/zh-CN.js +++ b/src/channels/web/static/i18n/zh-CN.js @@ -474,6 +474,10 @@ I18n.register('zh-CN', { 'cfg.routines_max_concurrent.desc': '同时运行的最大定时任务数', 'cfg.routines_cooldown.label': '默认冷却时间', 'cfg.routines_cooldown.desc': '定时任务触发间的最小秒数', + 'cfg.routines_full_job_default_mode.label': '完整任务默认权限模式', + 'cfg.routines_full_job_default_mode.desc': '新建 full_job 定时任务的默认权限行为。未设置时使用 inherit_owner。', + 'cfg.routines_full_job_owner_tools.label': '完整任务所有者允许工具', + 'cfg.routines_full_job_owner_tools.desc': '逗号分隔的工具名列表,full_job 定时任务可在运行时继承这些工具权限。', // 安全设置 'cfg.safety_max_output.label': '最大输出长度', diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index 861b5bd2..c8601fdd 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -884,9 +884,20 @@ pub struct RoutineDetailResponse { pub run_count: u64, pub consecutive_failures: u32, pub created_at: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub full_job_permissions: Option, pub recent_runs: Vec, } +#[derive(Debug, Serialize)] +pub struct FullJobPermissionInfo { + pub permission_mode: String, + pub default_permission_mode: String, + pub stored_tool_permissions: Vec, + pub owner_allowed_tools: Vec, + pub effective_tool_permissions: Vec, +} + #[derive(Debug, Serialize)] pub struct RoutineRunInfo { pub id: Uuid, diff --git a/src/service.rs b/src/service.rs index 679e6fe2..37fda696 100644 --- a/src/service.rs +++ b/src/service.rs @@ -94,6 +94,7 @@ fn macos_plist_content(exe: &str, stdout: &str, stderr: &str) -> String { KeepAlive + EnvironmentVariables CLI_ENABLED @@ -127,6 +128,7 @@ fn install_linux() -> Result<()> { \n\ [Service]\n\ Type=simple\n\ + # Disable interactive CLI/REPL in daemon mode to prevent blocking on stdin\n\ Environment=\"CLI_ENABLED=false\"\n\ ExecStart=\"{exe}\" run\n\ Restart=always\n\ diff --git a/src/tools/builtin/routine.rs b/src/tools/builtin/routine.rs index 22db7c74..6f440e0b 100644 --- a/src/tools/builtin/routine.rs +++ b/src/tools/builtin/routine.rs @@ -19,7 +19,9 @@ use serde_json::{Map, Value}; use uuid::Uuid; use crate::agent::routine::{ - NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, next_cron_fire, + FullJobPermissionDefaultMode, FullJobPermissionMode, NotifyConfig, Routine, RoutineAction, + RoutineGuardrails, Trigger, load_full_job_permission_settings, next_cron_fire, + normalize_tool_names, }; use crate::agent::routine_engine::RoutineEngine; use crate::context::JobContext; @@ -54,6 +56,13 @@ enum NormalizedExecutionMode { FullJob, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RequestedFullJobPermissionMode { + Explicit, + InheritOwner, + CopyOwner, +} + #[derive(Debug, Clone, PartialEq, Eq)] struct NormalizedExecutionRequest { mode: NormalizedExecutionMode, @@ -61,6 +70,7 @@ struct NormalizedExecutionRequest { use_tools: bool, max_tool_rounds: u32, tool_permissions: Vec, + permission_mode: Option, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -149,6 +159,11 @@ fn execution_properties() -> Value { "type": "array", "items": { "type": "string" }, "description": "Only applies when execution.mode='full_job'. These tools are pre-authorized for Always-approval checks." + }, + "permission_mode": { + "type": "string", + "enum": ["inherit_owner", "explicit", "copy_owner"], + "description": "Only applies when execution.mode='full_job'. 'inherit_owner' uses the owner defaults at run time, 'explicit' uses only tool_permissions, and 'copy_owner' snapshots the current owner allowlist into tool_permissions." } }) } @@ -321,7 +336,7 @@ fn lightweight_execution_variant() -> Value { fn full_job_execution_variant() -> Value { serde_json::json!({ "type": "object", - "description": "Full-job execution. Uses tool_permissions and ignores lightweight-only fields such as use_tools, max_tool_rounds, and context_paths.", + "description": "Full-job execution. Uses owner-scoped permission defaults plus tool_permissions and ignores lightweight-only fields such as use_tools, max_tool_rounds, and context_paths.", "properties": { "mode": { "type": "string", @@ -332,6 +347,11 @@ fn full_job_execution_variant() -> Value { "type": "array", "items": { "type": "string" }, "description": "Tools pre-authorized for Always-approval checks." + }, + "permission_mode": { + "type": "string", + "enum": ["inherit_owner", "explicit", "copy_owner"], + "description": "When omitted, new routines use the owner default. 'copy_owner' snapshots the current owner allowlist into this routine." } }, "required": ["mode"] @@ -349,7 +369,7 @@ fn execution_discovery_schema() -> Value { ], "examples": [ { "mode": "lightweight", "use_tools": true, "max_tool_rounds": 3 }, - { "mode": "full_job", "tool_permissions": ["message", "http"] } + { "mode": "full_job", "permission_mode": "inherit_owner", "tool_permissions": ["message", "http"] } ] }) } @@ -399,6 +419,7 @@ fn routine_create_examples() -> Vec { }, "execution": { "mode": "full_job", + "permission_mode": "inherit_owner", "tool_permissions": ["message"] } }), @@ -412,7 +433,7 @@ fn routine_create_tool_summary() -> ToolDiscoverySummary { "request.kind='cron' requires request.schedule.".into(), "request.kind='message_event' requires request.pattern.".into(), "request.kind='system_event' requires request.source and request.event_type.".into(), - "execution.mode='full_job' uses tool_permissions and ignores use_tools, max_tool_rounds, and context_paths.".into(), + "execution.mode='full_job' uses permission_mode and tool_permissions, and ignores use_tools, max_tool_rounds, and context_paths.".into(), ], notes: vec![ "Omitting execution defaults to lightweight mode.".into(), @@ -577,6 +598,14 @@ fn routine_create_schema(include_compatibility_aliases: bool) -> Value { "description": "Compatibility alias for execution.tool_permissions." }), ); + properties.insert( + "permission_mode".to_string(), + serde_json::json!({ + "type": "string", + "enum": ["inherit_owner", "explicit", "copy_owner"], + "description": "Compatibility alias for execution.permission_mode." + }), + ); properties.insert( "notify_channel".to_string(), serde_json::json!({ @@ -655,6 +684,16 @@ pub(crate) fn routine_update_parameters_schema() -> Value { "description": { "type": "string", "description": "New description" + }, + "tool_permissions": { + "type": "array", + "items": { "type": "string" }, + "description": "Updated Always-approval tool allowlist for full_job routines only." + }, + "permission_mode": { + "type": "string", + "enum": ["inherit_owner", "explicit", "copy_owner"], + "description": "Updated permission mode for full_job routines only. 'copy_owner' snapshots the current owner allowlist into the routine and persists as explicit." } }, "required": ["name"] @@ -700,6 +739,27 @@ fn u64_field(params: &Value, group: &str, field: &str, aliases: &[&str]) -> Opti } fn string_array_field(params: &Value, group: &str, field: &str, aliases: &[&str]) -> Vec { + normalize_tool_names( + nested_object(params, group) + .and_then(|obj| obj.get(field)) + .and_then(Value::as_array) + .or_else(|| { + aliases + .iter() + .find_map(|alias| params.get(*alias).and_then(Value::as_array)) + }) + .into_iter() + .flatten() + .filter_map(|value| value.as_str().map(String::from)), + ) +} + +fn optional_string_array_field( + params: &Value, + group: &str, + field: &str, + aliases: &[&str], +) -> Option> { nested_object(params, group) .and_then(|obj| obj.get(field)) .and_then(Value::as_array) @@ -709,11 +769,11 @@ fn string_array_field(params: &Value, group: &str, field: &str, aliases: &[&str] .find_map(|alias| params.get(*alias).and_then(Value::as_array)) }) .map(|arr| { - arr.iter() - .filter_map(|value| value.as_str().map(String::from)) - .collect() + normalize_tool_names( + arr.iter() + .filter_map(|value| value.as_str().map(String::from)), + ) }) - .unwrap_or_default() } fn object_field( @@ -852,6 +912,20 @@ fn parse_execution_mode(value: Option) -> Result, +) -> Result, ToolError> { + match value.as_deref() { + None => Ok(None), + Some("explicit") => Ok(Some(RequestedFullJobPermissionMode::Explicit)), + Some("inherit_owner") => Ok(Some(RequestedFullJobPermissionMode::InheritOwner)), + Some("copy_owner") => Ok(Some(RequestedFullJobPermissionMode::CopyOwner)), + Some(other) => Err(ToolError::InvalidParameters(format!( + "unknown full_job permission_mode: {other}" + ))), + } +} + fn parse_routine_execution(params: &Value) -> Result { let mode = parse_execution_mode(string_field(params, "execution", "mode", &["action_type"]))?; let context_paths = @@ -867,6 +941,12 @@ fn parse_routine_execution(params: &Value) -> Result Result Trigger { } } -fn build_routine_action( +async fn build_routine_action( + store: &dyn Database, + user_id: &str, name: &str, prompt: &str, execution: &NormalizedExecutionRequest, -) -> RoutineAction { +) -> Result { match execution.mode { - NormalizedExecutionMode::Lightweight => RoutineAction::Lightweight { + NormalizedExecutionMode::Lightweight => Ok(RoutineAction::Lightweight { prompt: prompt.to_string(), context_paths: execution.context_paths.clone(), max_tokens: 4096, use_tools: execution.use_tools, max_tool_rounds: execution.max_tool_rounds, - }, - NormalizedExecutionMode::FullJob => RoutineAction::FullJob { - title: name.to_string(), - description: prompt.to_string(), - max_iterations: 10, - tool_permissions: execution.tool_permissions.clone(), - }, + }), + NormalizedExecutionMode::FullJob => { + let mut owner_settings = None; + let requested_mode = match execution.permission_mode { + Some(mode) => mode, + None => { + let settings = load_full_job_permission_settings(store, user_id) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!( + "failed to load routine permission settings: {e}" + )) + })?; + let mode = match settings.default_mode { + FullJobPermissionDefaultMode::Explicit => { + RequestedFullJobPermissionMode::Explicit + } + FullJobPermissionDefaultMode::InheritOwner => { + RequestedFullJobPermissionMode::InheritOwner + } + FullJobPermissionDefaultMode::CopyOwner => { + RequestedFullJobPermissionMode::CopyOwner + } + }; + owner_settings = Some(settings); + mode + } + }; + let (permission_mode, tool_permissions) = match requested_mode { + RequestedFullJobPermissionMode::Explicit => ( + FullJobPermissionMode::Explicit, + execution.tool_permissions.clone(), + ), + RequestedFullJobPermissionMode::InheritOwner => ( + FullJobPermissionMode::InheritOwner, + execution.tool_permissions.clone(), + ), + RequestedFullJobPermissionMode::CopyOwner => { + let owner_allowed_tools = match owner_settings { + Some(settings) => settings.owner_allowed_tools, + None => { + load_full_job_permission_settings(store, user_id) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!( + "failed to load routine permission settings: {e}" + )) + })? + .owner_allowed_tools + } + }; + ( + FullJobPermissionMode::Explicit, + normalize_tool_names( + owner_allowed_tools + .into_iter() + .chain(execution.tool_permissions.iter().cloned()), + ), + ) + } + }; + Ok(RoutineAction::FullJob { + title: name.to_string(), + description: prompt.to_string(), + max_iterations: 10, + tool_permissions, + permission_mode, + }) + } } } +fn routine_requests_full_job(params: &Value) -> bool { + matches!( + string_field(params, "execution", "mode", &["action_type"]).as_deref(), + Some("full_job") + ) +} + +fn routine_permission_fields_present(params: &Value) -> bool { + nested_object(params, "execution").is_some_and(|execution| { + execution.contains_key("tool_permissions") || execution.contains_key("permission_mode") + }) || params.get("tool_permissions").is_some() + || params.get("permission_mode").is_some() +} + fn event_emit_schema(include_source_alias: bool) -> Value { let mut schema = serde_json::json!({ "type": "object", @@ -1054,6 +1213,14 @@ impl Tool for RoutineCreateTool { Use this when the user wants something to happen periodically or reactively." } + fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement { + if routine_requests_full_job(params) { + ApprovalRequirement::UnlessAutoApproved + } else { + ApprovalRequirement::Never + } + } + fn parameters_schema(&self) -> serde_json::Value { routine_create_parameters_schema() } @@ -1074,8 +1241,14 @@ impl Tool for RoutineCreateTool { let start = std::time::Instant::now(); let normalized = parse_routine_create_request(¶ms)?; let trigger = build_routine_trigger(&normalized.trigger); - let action = - build_routine_action(&normalized.name, &normalized.prompt, &normalized.execution); + let action = build_routine_action( + self.store.as_ref(), + &ctx.user_id, + &normalized.name, + &normalized.prompt, + &normalized.execution, + ) + .await?; // Compute next fire time for cron let next_fire = if let Trigger::Cron { @@ -1238,14 +1411,23 @@ impl Tool for RoutineUpdateTool { } fn description(&self) -> &str { - "Update an existing routine. Can change prompt, description, enabled state, or cron schedule/timezone. \ - Pass the routine name and only the fields you want to change. This does not convert trigger types." + "Update an existing routine. Can change prompt, description, enabled state, cron schedule/timezone, \ + or full_job permission settings. Pass the routine name and only the fields you want to change. \ + This does not convert trigger types." } fn parameters_schema(&self) -> serde_json::Value { routine_update_parameters_schema() } + fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement { + if routine_permission_fields_present(params) { + ApprovalRequirement::UnlessAutoApproved + } else { + ApprovalRequirement::Never + } + } + async fn execute( &self, params: serde_json::Value, @@ -1278,6 +1460,72 @@ impl Tool for RoutineUpdateTool { } } + let requested_permission_mode = parse_requested_full_job_permission_mode(string_field( + ¶ms, + "execution", + "permission_mode", + &["permission_mode"], + ))?; + let requested_tool_permissions = optional_string_array_field( + ¶ms, + "execution", + "tool_permissions", + &["tool_permissions"], + ); + let updates_permissions = + requested_permission_mode.is_some() || requested_tool_permissions.is_some(); + + if updates_permissions { + match &mut routine.action { + RoutineAction::FullJob { + tool_permissions, + permission_mode, + .. + } => { + let next_tool_permissions = + requested_tool_permissions.unwrap_or_else(|| tool_permissions.clone()); + match requested_permission_mode { + Some(RequestedFullJobPermissionMode::Explicit) => { + *permission_mode = FullJobPermissionMode::Explicit; + *tool_permissions = next_tool_permissions; + } + Some(RequestedFullJobPermissionMode::InheritOwner) => { + *permission_mode = FullJobPermissionMode::InheritOwner; + *tool_permissions = next_tool_permissions; + } + Some(RequestedFullJobPermissionMode::CopyOwner) => { + let owner_settings = load_full_job_permission_settings( + self.store.as_ref(), + &ctx.user_id, + ) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!( + "failed to load routine permission settings: {e}" + )) + })?; + *permission_mode = FullJobPermissionMode::Explicit; + *tool_permissions = normalize_tool_names( + owner_settings + .owner_allowed_tools + .into_iter() + .chain(next_tool_permissions), + ); + } + None => { + *tool_permissions = next_tool_permissions; + } + } + } + RoutineAction::Lightweight { .. } => { + return Err(ToolError::InvalidParameters( + "permission_mode and tool_permissions can only be updated for full_job routines" + .to_string(), + )); + } + } + } + // Validate timezone param if provided let new_timezone = params .get("timezone") @@ -1686,6 +1934,7 @@ mod tests { "use_tools", "max_tool_rounds", "tool_permissions", + "permission_mode", "notify_channel", "notify_user", "cooldown_secs", @@ -1814,6 +2063,7 @@ mod tests { parsed.execution.tool_permissions, vec!["message".to_string(), "http".to_string()], ); + assert_eq!(parsed.execution.permission_mode, None); assert_eq!(parsed.delivery.channel.as_deref(), Some("telegram")); assert_eq!(parsed.delivery.user.as_deref(), Some("ops-team")); assert_eq!(parsed.cooldown_secs, 30); @@ -2143,8 +2393,9 @@ mod tests { .and_then(Value::as_object) .expect("full_job properties"); assert!( - full_job_props.contains_key("tool_permissions"), - "full_job variant should expose tool_permissions", + full_job_props.contains_key("tool_permissions") + && full_job_props.contains_key("permission_mode"), + "full_job variant should expose permission fields", ); } @@ -2249,6 +2500,8 @@ mod tests { "schedule", "timezone", "description", + "tool_permissions", + "permission_mode", ] { let _ = schema_property(&schema, field); } @@ -2272,6 +2525,24 @@ mod tests { ); } + #[test] + fn routine_create_detects_full_job_requests_for_approval() { + let full_job = serde_json::json!({ + "name": "approve-me", + "prompt": "Run autonomously", + "request": { "kind": "manual" }, + "execution": { "mode": "full_job" } + }); + let lightweight = serde_json::json!({ + "name": "safe", + "prompt": "Stay lightweight", + "request": { "kind": "manual" } + }); + + assert!(routine_requests_full_job(&full_job)); + assert!(!routine_requests_full_job(&lightweight)); + } + #[test] fn event_emit_parameters_schema_prefers_canonical_event_source() { let schema = event_emit_parameters_schema(); @@ -2312,4 +2583,72 @@ mod tests { "event_emit discovery schema should keep source alias", ); } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn build_full_job_action_defaults_to_inherit_owner_for_new_routines() { + let (db, _tmp) = crate::testing::test_db().await; + let execution = NormalizedExecutionRequest { + mode: NormalizedExecutionMode::FullJob, + context_paths: Vec::new(), + use_tools: false, + max_tool_rounds: 3, + tool_permissions: vec!["shell".to_string()], + permission_mode: None, + }; + + let action = + build_routine_action(db.as_ref(), "default", "issue-1316", "Run it", &execution) + .await + .expect("build action"); + + assert!(matches!( + action, + RoutineAction::FullJob { + permission_mode: FullJobPermissionMode::InheritOwner, + tool_permissions, + .. + } if tool_permissions == vec!["shell".to_string()] + )); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn build_full_job_action_copy_owner_snapshots_allowlist() { + let (db, _tmp) = crate::testing::test_db().await; + db.set_setting( + "default", + crate::agent::routine::FULL_JOB_OWNER_ALLOWED_TOOLS_SETTING_KEY, + &serde_json::json!(["http", "shell"]), + ) + .await + .expect("set owner allowlist"); + let execution = NormalizedExecutionRequest { + mode: NormalizedExecutionMode::FullJob, + context_paths: Vec::new(), + use_tools: false, + max_tool_rounds: 3, + tool_permissions: vec!["message".to_string(), "shell".to_string()], + permission_mode: Some(RequestedFullJobPermissionMode::CopyOwner), + }; + + let action = + build_routine_action(db.as_ref(), "default", "issue-1316", "Run it", &execution) + .await + .expect("build action"); + + assert!(matches!( + action, + RoutineAction::FullJob { + permission_mode: FullJobPermissionMode::Explicit, + tool_permissions, + .. + } if tool_permissions + == vec![ + "http".to_string(), + "shell".to_string(), + "message".to_string(), + ] + )); + } } diff --git a/tests/dispatched_routine_run_tests.rs b/tests/dispatched_routine_run_tests.rs index 4ab5d2a8..e5024570 100644 --- a/tests/dispatched_routine_run_tests.rs +++ b/tests/dispatched_routine_run_tests.rs @@ -15,7 +15,8 @@ mod tests { use uuid::Uuid; use ironclaw::agent::routine::{ - Routine, RoutineAction, RoutineGuardrails, RoutineRun, RunStatus, Trigger, + FullJobPermissionMode, Routine, RoutineAction, RoutineGuardrails, RoutineRun, RunStatus, + Trigger, }; use ironclaw::context::{JobContext, JobState}; use ironclaw::db::Database; @@ -46,6 +47,7 @@ mod tests { description: "Test description".to_string(), max_iterations: 5, tool_permissions: vec![], + permission_mode: FullJobPermissionMode::Explicit, }, guardrails: RoutineGuardrails { cooldown: std::time::Duration::from_secs(0), diff --git a/tests/e2e_builtin_tool_coverage.rs b/tests/e2e_builtin_tool_coverage.rs index d08f2204..03c1aefe 100644 --- a/tests/e2e_builtin_tool_coverage.rs +++ b/tests/e2e_builtin_tool_coverage.rs @@ -10,7 +10,7 @@ mod support; mod tests { use std::time::Duration; - use ironclaw::agent::routine::{RoutineAction, Trigger}; + use ironclaw::agent::routine::{FullJobPermissionMode, RoutineAction, Trigger}; use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; @@ -359,10 +359,12 @@ mod tests { RoutineAction::FullJob { description, tool_permissions, + permission_mode, .. } => { assert!(description.contains("Summarize the new issue")); assert_eq!(tool_permissions, &vec!["shell".to_string()]); + assert_eq!(permission_mode, &FullJobPermissionMode::InheritOwner); } other => panic!("expected full_job action, got {other:?}"), } @@ -413,6 +415,7 @@ mod tests { RoutineAction::FullJob { description, tool_permissions, + permission_mode, .. } => { assert!(description.contains("Prepare the morning digest")); @@ -420,6 +423,7 @@ mod tests { tool_permissions, &vec!["message".to_string(), "http".to_string()] ); + assert_eq!(permission_mode, &FullJobPermissionMode::InheritOwner); } other => panic!("expected full_job action, got {other:?}"), } diff --git a/tests/e2e_routine_heartbeat.rs b/tests/e2e_routine_heartbeat.rs index 25432f3d..116dd1e0 100644 --- a/tests/e2e_routine_heartbeat.rs +++ b/tests/e2e_routine_heartbeat.rs @@ -12,35 +12,107 @@ mod tests { use std::time::Duration; use chrono::Utc; + use libsql::params; use uuid::Uuid; use ironclaw::agent::routine::{ - NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, + FullJobPermissionMode, NotifyConfig, Routine, RoutineAction, RoutineGuardrails, RoutineRun, + RunStatus, Trigger, }; use ironclaw::agent::routine_engine::RoutineEngine; - use ironclaw::agent::{HeartbeatConfig, HeartbeatRunner}; + use ironclaw::agent::{HeartbeatConfig, HeartbeatRunner, Scheduler}; use ironclaw::channels::IncomingMessage; - use ironclaw::config::{RoutineConfig, SafetyConfig}; - use ironclaw::db::Database; + use ironclaw::config::{AgentConfig, RoutineConfig, SafetyConfig}; + use ironclaw::context::{ContextManager, JobContext}; + use ironclaw::db::{Database, libsql::LibSqlBackend}; + use ironclaw::hooks::HookRegistry; + use ironclaw::llm::LlmProvider; use ironclaw::safety::SafetyLayer; - use ironclaw::tools::ToolRegistry; + use ironclaw::tools::builtin::routine::RoutineUpdateTool; + use ironclaw::tools::{ApprovalRequirement, Tool, ToolError, ToolOutput, ToolRegistry}; use ironclaw::workspace::Workspace; use ironclaw::workspace::hygiene::HygieneConfig; - use crate::support::trace_llm::{LlmTrace, TraceLlm, TraceResponse, TraceStep}; + use crate::support::trace_llm::{LlmTrace, TraceLlm, TraceResponse, TraceStep, TraceToolCall}; + + const OWNER_GATE_COUNT_SETTING_KEY: &str = "tests.owner_gate_count"; + + struct OwnerGateTool { + store: Arc, + } + + #[async_trait::async_trait] + impl Tool for OwnerGateTool { + fn name(&self) -> &str { + "owner_gate" + } + + fn description(&self) -> &str { + "Test-only tool gated by owner full_job permissions" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": {} + }) + } + + async fn execute( + &self, + _params: serde_json::Value, + ctx: &JobContext, + ) -> Result { + let start = std::time::Instant::now(); + let current = self + .store + .get_setting(&ctx.user_id, OWNER_GATE_COUNT_SETTING_KEY) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!("failed to read owner gate count: {e}")) + })? + .and_then(|value| value.as_i64()) + .unwrap_or(0); + self.store + .set_setting( + &ctx.user_id, + OWNER_GATE_COUNT_SETTING_KEY, + &serde_json::json!(current + 1), + ) + .await + .map_err(|e| { + ToolError::ExecutionFailed(format!("failed to persist owner gate count: {e}")) + })?; + + Ok(ToolOutput::text("owner gate executed", start.elapsed())) + } + + fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { + ApprovalRequirement::Always + } + + fn requires_sanitization(&self) -> bool { + false + } + } /// Create a temp libSQL database with migrations applied. async fn create_test_db() -> (Arc, tempfile::TempDir) { - use ironclaw::db::libsql::LibSqlBackend; + let (backend, temp_dir) = create_test_backend().await; + let db: Arc = backend; + (db, temp_dir) + } + async fn create_test_backend() -> (Arc, tempfile::TempDir) { let temp_dir = tempfile::tempdir().expect("tempdir"); let db_path = temp_dir.path().join("test.db"); - let backend = LibSqlBackend::new_local(&db_path) - .await - .expect("LibSqlBackend"); + let backend = Arc::new( + LibSqlBackend::new_local(&db_path) + .await + .expect("LibSqlBackend"), + ); backend.run_migrations().await.expect("migrations"); - let db: Arc = Arc::new(backend); - (db, temp_dir) + (backend, temp_dir) } /// Create a workspace backed by the test database. @@ -93,6 +165,143 @@ mod tests { } } + fn make_full_job_routine( + name: &str, + permission_mode: FullJobPermissionMode, + tool_permissions: Vec, + ) -> Routine { + Routine { + id: Uuid::new_v4(), + name: name.to_string(), + description: format!("Full-job test routine: {name}"), + user_id: "default".to_string(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::FullJob { + title: name.to_string(), + description: "Use the owner-gated tool when permitted.".to_string(), + max_iterations: 3, + tool_permissions, + permission_mode, + }, + 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(), + } + } + + fn owner_gate_trace(include_completion: bool) -> LlmTrace { + let mut steps = vec![TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_owner_gate".to_string(), + name: "owner_gate".to_string(), + arguments: serde_json::json!({}), + }], + input_tokens: 40, + output_tokens: 10, + }, + expected_tool_results: vec![], + }]; + if include_completion { + // The worker first calls `select_tools()`, then falls back to + // `respond_with_tools()` when no tool calls are returned. Both + // methods consume a trace step, so the successful completion path + // needs two text responses after the tool call. + for _ in 0..2 { + steps.push(TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "I have completed the task.".to_string(), + input_tokens: 20, + output_tokens: 5, + }, + expected_tool_results: vec![], + }); + } + } + LlmTrace::single_turn("test-owner-gate", "run owner gate", steps) + } + + async fn setup_owner_gate_engine(db: Arc, trace: LlmTrace) -> Arc { + let ws = create_workspace(&db); + let (notify_tx, _rx) = tokio::sync::mpsc::channel(16); + let registry = Arc::new(ToolRegistry::new()); + registry + .register(Arc::new(OwnerGateTool { store: db.clone() })) + .await; + + let safety = Arc::new(SafetyLayer::new(&SafetyConfig { + max_output_length: 100_000, + injection_check_enabled: false, + })); + let llm: Arc = Arc::new(TraceLlm::from_trace(trace)); + let scheduler = Arc::new(Scheduler::new( + AgentConfig::for_testing(), + Arc::new(ContextManager::new(5)), + llm.clone(), + safety.clone(), + registry.clone(), + Some(db.clone()), + Arc::new(HookRegistry::new()), + )); + + Arc::new(RoutineEngine::new( + RoutineConfig::default(), + db, + llm, + ws, + notify_tx, + Some(scheduler), + registry, + safety, + )) + } + + async fn owner_gate_count(db: &Arc) -> i64 { + db.get_setting("default", OWNER_GATE_COUNT_SETTING_KEY) + .await + .expect("get owner gate count") + .and_then(|value| value.as_i64()) + .unwrap_or(0) + } + + async fn wait_for_run_completion( + db: &Arc, + routine_id: Uuid, + run_id: Uuid, + ) -> RoutineRun { + let deadline = std::time::Instant::now() + Duration::from_secs(10); + loop { + let runs = db + .list_routine_runs(routine_id, 10) + .await + .expect("list_routine_runs"); + if let Some(run) = runs.into_iter().find(|run| run.id == run_id) + && run.status != RunStatus::Running + { + return run; + } + + assert!( + std::time::Instant::now() < deadline, + "timed out waiting for routine run {run_id} to complete" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + } + // ----------------------------------------------------------------------- // Test 1: cron_routine_fires // ----------------------------------------------------------------------- @@ -884,6 +1093,7 @@ mod tests { description: "d".to_string(), max_iterations: 3, tool_permissions: vec![], + permission_mode: ironclaw::agent::routine::FullJobPermissionMode::Explicit, }, guardrails: RoutineGuardrails { cooldown: Duration::from_secs(0), @@ -1029,4 +1239,153 @@ mod tests { "cron routine should fire after global slot is released" ); } + + // ----------------------------------------------------------------------- + // Test: inherit_owner full_job routines can use owner-gated tools + // ----------------------------------------------------------------------- + + #[tokio::test] + async fn full_job_inherit_owner_uses_owner_allowlist() { + let (backend, _tmp) = create_test_backend().await; + let db: Arc = backend; + let engine = setup_owner_gate_engine(db.clone(), owner_gate_trace(true)).await; + + db.set_setting( + "default", + ironclaw::agent::routine::FULL_JOB_OWNER_ALLOWED_TOOLS_SETTING_KEY, + &serde_json::json!(["owner_gate"]), + ) + .await + .expect("set owner allowlist"); + + let routine = make_full_job_routine( + "inherit-owner-allowed", + FullJobPermissionMode::InheritOwner, + vec![], + ); + db.create_routine(&routine).await.expect("create_routine"); + + let run_id = engine + .fire_manual(routine.id, None) + .await + .expect("fire manual"); + let run = wait_for_run_completion(&db, routine.id, run_id).await; + + assert_eq!(run.status, RunStatus::Ok); + assert_eq!(owner_gate_count(&db).await, 1); + } + + // ----------------------------------------------------------------------- + // Test: inherit_owner full_job routines stay blocked without owner allowlist + // ----------------------------------------------------------------------- + + #[tokio::test] + async fn full_job_inherit_owner_blocks_without_owner_allowlist() { + let (backend, _tmp) = create_test_backend().await; + let db: Arc = backend; + let engine = setup_owner_gate_engine(db.clone(), owner_gate_trace(false)).await; + + let routine = make_full_job_routine( + "inherit-owner-blocked", + FullJobPermissionMode::InheritOwner, + vec![], + ); + db.create_routine(&routine).await.expect("create_routine"); + + let run_id = engine + .fire_manual(routine.id, None) + .await + .expect("fire manual"); + let run = wait_for_run_completion(&db, routine.id, run_id).await; + + assert_eq!(run.status, RunStatus::Failed); + assert_eq!(owner_gate_count(&db).await, 0); + } + + // ----------------------------------------------------------------------- + // Test: legacy full_job routines remain explicit until updated + // ----------------------------------------------------------------------- + + #[tokio::test] + async fn legacy_full_job_stays_explicit_until_updated() { + let (backend, _tmp) = create_test_backend().await; + let db: Arc = backend.clone(); + + db.set_setting( + "default", + ironclaw::agent::routine::FULL_JOB_OWNER_ALLOWED_TOOLS_SETTING_KEY, + &serde_json::json!(["owner_gate"]), + ) + .await + .expect("set owner allowlist"); + + let legacy_routine = + make_full_job_routine("legacy-full-job", FullJobPermissionMode::Explicit, vec![]); + db.create_routine(&legacy_routine) + .await + .expect("create_routine"); + + let conn = backend.connect().await.expect("connect"); + conn.execute( + "UPDATE routines SET action_config = ?1 WHERE id = ?2", + params![ + serde_json::json!({ + "title": legacy_routine.name, + "description": "Use the owner-gated tool when permitted.", + "max_iterations": 3, + "tool_permissions": [], + }) + .to_string(), + legacy_routine.id.to_string(), + ], + ) + .await + .expect("strip permission_mode from action_config"); + + let blocked_engine = setup_owner_gate_engine(db.clone(), owner_gate_trace(false)).await; + let first_run_id = blocked_engine + .fire_manual(legacy_routine.id, None) + .await + .expect("fire manual legacy routine"); + let first_run = wait_for_run_completion(&db, legacy_routine.id, first_run_id).await; + + assert_eq!(first_run.status, RunStatus::Failed); + assert_eq!(owner_gate_count(&db).await, 0); + + let update_tool = RoutineUpdateTool::new(db.clone(), blocked_engine.clone()); + let update_ctx = JobContext::with_user("default", "update", "update legacy routine"); + update_tool + .execute( + serde_json::json!({ + "name": legacy_routine.name, + "permission_mode": "inherit_owner", + }), + &update_ctx, + ) + .await + .expect("routine_update should succeed"); + + let updated = db + .get_routine(legacy_routine.id) + .await + .expect("get_routine") + .expect("routine should still exist"); + assert!(matches!( + updated.action, + RoutineAction::FullJob { + permission_mode: FullJobPermissionMode::InheritOwner, + .. + } + )); + + let allowed_engine = setup_owner_gate_engine(db.clone(), owner_gate_trace(true)).await; + let second_run_id = allowed_engine + .fire_manual(legacy_routine.id, None) + .await + .expect("fire manual updated routine"); + let second_run = wait_for_run_completion(&db, legacy_routine.id, second_run_id).await; + + assert_eq!(second_run.status, RunStatus::Ok); + assert_eq!(owner_gate_count(&db).await, 1); + } } diff --git a/tests/gateway_workflow_integration.rs b/tests/gateway_workflow_integration.rs index 187cc751..e6aeca9c 100644 --- a/tests/gateway_workflow_integration.rs +++ b/tests/gateway_workflow_integration.rs @@ -13,6 +13,10 @@ mod support; mod tests { use std::time::Duration; + use chrono::Utc; + use ironclaw::agent::routine::{ + FullJobPermissionMode, NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, + }; use uuid::Uuid; use crate::support::gateway_workflow_harness::GatewayWorkflowHarness; @@ -260,4 +264,106 @@ mod tests { harness.shutdown().await; mock.shutdown().await; } + + #[tokio::test] + async fn routines_detail_exposes_full_job_permission_resolution() { + 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; + + harness + .db + .set_setting( + &harness.user_id, + ironclaw::agent::routine::FULL_JOB_OWNER_ALLOWED_TOOLS_SETTING_KEY, + &serde_json::json!(["shell", "http"]), + ) + .await + .expect("set owner allowlist"); + harness + .db + .set_setting( + &harness.user_id, + ironclaw::agent::routine::FULL_JOB_DEFAULT_PERMISSION_MODE_SETTING_KEY, + &serde_json::json!("copy_owner"), + ) + .await + .expect("set owner default mode"); + + let routine = Routine { + id: Uuid::new_v4(), + name: "wf-full-job-permissions".to_string(), + description: "Permission detail regression test".to_string(), + user_id: harness.user_id.clone(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::FullJob { + title: "permission-detail".to_string(), + description: "Check effective permission detail".to_string(), + max_iterations: 3, + tool_permissions: vec!["message".to_string()], + permission_mode: FullJobPermissionMode::InheritOwner, + }, + 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(), + }; + harness + .db + .create_routine(&routine) + .await + .expect("create routine"); + + 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["full_job_permissions"]["permission_mode"].as_str(), + Some("inherit_owner") + ); + assert_eq!( + detail["full_job_permissions"]["default_permission_mode"].as_str(), + Some("copy_owner") + ); + assert_eq!( + detail["full_job_permissions"]["owner_allowed_tools"], + serde_json::json!(["shell", "http"]) + ); + assert_eq!( + detail["full_job_permissions"]["effective_tool_permissions"], + serde_json::json!(["shell", "http", "message"]) + ); + + harness.shutdown().await; + mock.shutdown().await; + } } From 6b0f84bbe04edbfab2c8f0c5cda13c818e195dcc Mon Sep 17 00:00:00 2001 From: Henry Park Date: Thu, 19 Mar 2026 18:33:04 -0700 Subject: [PATCH 2/3] perf: use Arc in embedding cache to avoid clones on miss path (#1438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs: add comments explaining CLI_ENABLED=false in service templates (#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes #990 Co-Authored-By: Claude Opus 4.6 (1M context) * perf: use Arc> in embedding cache to avoid clones on miss path (#1429) Store embeddings as Arc> internally so that cache insertions share the allocation with the return value via Arc::clone instead of cloning the entire float vector (6-12 KB per embedding). - embed() miss path: Arc::try_unwrap avoids a clone when returning (the cache holds one Arc ref, the return path holds the other; try_unwrap succeeds when the thundering-herd path doesn't fire) - embed_batch() miss path: cache first via Arc::clone, then try_unwrap for results — embeddings skipped due to capacity limits are returned without any clone - Hit path still clones (trait returns Vec); a future trait change to Arc> could eliminate this too Closes #1429 Co-Authored-By: Claude Opus 4.6 (1M context) * style: fix formatting in embedding_cache.rs Co-Authored-By: Claude Opus 4.6 (1M context) * fix: address PR review — correct doc comment and remove dead try_unwrap - Reword CacheEntry doc comment to accurately reflect that hit/miss paths still clone into a fresh Vec for callers; Arc sharing only helps in embed_batch when embeddings are skipped from caching - Remove Arc::try_unwrap in embed() which could never succeed (cache always holds an Arc ref, so refcount >= 2) Co-Authored-By: Claude Opus 4.6 (1M context) * fix: revert embed() to plain Vec, keep Arc only in embed_batch() In embed(), Arc adds overhead (allocation + refcount) without saving any clones — the original pattern (clone for cache, return by move) was already optimal. Arc only helps in embed_batch() where capacity-skipped embeddings can be returned via try_unwrap. Co-Authored-By: Claude Opus 4.6 (1M context) * fix: move clone+Arc::new outside mutex in embed() Clone the embedding and wrap in Arc before acquiring the lock so the mutex is held only for the HashMap insert, not during the O(n) copy. Co-Authored-By: Claude Opus 4.6 (1M context) * refactor: drop Arc, use cache-then-move pattern instead Arc was the wrong abstraction — the trait returns Vec, so Arc can't avoid clones on return paths. Instead: - embed(): skip clone in thundering-herd case (just touch timestamp) - embed_batch(): cache first (clone only cacheable subset), then move originals into results (zero-copy). For N misses with K cacheable: old = 2N clones, new = K clones. - CacheEntry reverted to plain Vec, no Arc overhead Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/workspace/embedding_cache.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/workspace/embedding_cache.rs b/src/workspace/embedding_cache.rs index 848bd2e5..21d3c7c3 100644 --- a/src/workspace/embedding_cache.rs +++ b/src/workspace/embedding_cache.rs @@ -183,8 +183,8 @@ impl EmbeddingProvider for CachedEmbeddingProvider { { let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); if let Some(entry) = guard.get_mut(&key) { - // Key already present (thundering herd) — just update, no eviction needed. - entry.embedding = embedding.clone(); + // Thundering herd — another caller already cached it. + // Just touch timestamp; skip the clone. entry.last_accessed = Instant::now(); } else { Self::evict_lru(&mut guard, self.config.max_entries); @@ -260,15 +260,10 @@ impl EmbeddingProvider for CachedEmbeddingProvider { "embedding batch: partial cache" ); - // Assemble results first (all misses, regardless of cache capacity). - for (orig_idx, emb) in miss_indices.iter().copied().zip(&new_embeddings) { - results[orig_idx] = Some(emb.clone()); - } - - // Cache the new embeddings, respecting max_entries. + // Cache FIRST (clone only the cacheable subset), then move originals + // into results. This avoids cloning capacity-skipped embeddings entirely. { let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - // When misses exceed capacity, clear and only cache the tail. let cacheable = miss_indices.len().min(self.config.max_entries); let skip = miss_indices.len() - cacheable; let need_to_evict = (guard.len() + cacheable).saturating_sub(self.config.max_entries); @@ -287,6 +282,11 @@ impl EmbeddingProvider for CachedEmbeddingProvider { } } + // Move originals into results (zero-copy for all, including cached ones). + for (orig_idx, emb) in miss_indices.iter().copied().zip(new_embeddings) { + results[orig_idx] = Some(emb); + } + results .into_iter() .enumerate() From 8920322589143822cec05415be025435f25be6d4 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Thu, 19 Mar 2026 18:33:15 -0700 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20staging=20CI=20triage=20=E2=80=94=20?= =?UTF-8?q?consolidate=20retry=20parsing,=20fix=20flaky=20tests,=20add=20d?= =?UTF-8?q?ocs=20(#1427)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: consolidate retry-after parsing and fix flaky OAuth env tests (#1288, #1280) - Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs - Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX pattern in oauth_defaults.rs) - Add regression tests for parse_retry_after edge cases Closes #1288, #1280 Co-Authored-By: Claude Opus 4.6 (1M context) * docs: add comments explaining CLI_ENABLED=false in service templates (#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes #990 Co-Authored-By: Claude Opus 4.6 (1M context) * fix: address review comments on retry-after consolidation - Change parse_retry_after() return type from Option to Duration (it never returns None due to the 60s fallback) - Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822 - Add parse_retry_after_http_date test for the RFC 2822 date parsing branch - Remove stale per-file test helpers (parse_retry_after_*_for_test) that duplicated old inline logic instead of testing the shared function - Remove unnecessary comments above #[cfg(test)] imports - Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in oauth_helpers tests to prevent cross-module env-var races Co-Authored-By: Claude Opus 4.6 (1M context) * fix: reword await_holding_lock safety comment Drop runtime-flavor assumption; justify by short-lived awaited operation. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- docs/plans/2026-03-18-staging-ci-triage.md | 87 ++++++++++++ src/llm/anthropic_oauth.rs | 94 +------------ src/llm/nearai_chat.rs | 148 +-------------------- src/llm/oauth_helpers.rs | 29 +++- src/llm/retry.rs | 76 +++++++++++ src/workspace/embeddings.rs | 69 +--------- 6 files changed, 201 insertions(+), 302 deletions(-) create mode 100644 docs/plans/2026-03-18-staging-ci-triage.md diff --git a/docs/plans/2026-03-18-staging-ci-triage.md b/docs/plans/2026-03-18-staging-ci-triage.md new file mode 100644 index 00000000..adfd5d05 --- /dev/null +++ b/docs/plans/2026-03-18-staging-ci-triage.md @@ -0,0 +1,87 @@ +# Staging CI Review Issues Triage + +**Date:** 2026-03-18 +**Branch:** staging (HEAD `b7a1edf`) +**Total open issues:** 50 + +--- + +## Batch 1 — Critical & 100-confidence issues + +| # | Title | Severity | Verdict | File(s) | Action | +|---|-------|----------|---------|---------|--------| +| 1281 | Logic inversion in Telegram auto-verification | CRITICAL:100 | **FALSE POSITIVE** (closed) | `src/channels/web/server.rs` | Different handlers with intentional different SSE behavior | +| 908 | Missing consecutive_failures reset | CRITICAL:100 | **STALE** | `src/llm/circuit_breaker.rs` | Close — `record_success()` already resets to 0 | +| 1282 | Variable shadowing fallback notification | HIGH:100 | **STALE** | `src/agent/agent_loop.rs` | Close — fixed in commit `bcc38ce` | +| 1283 | Inconsistent fallback logic DRY | HIGH:75 | **STALE** | `src/agent/agent_loop.rs` | Close — fixed in commit `bcc38ce` | +| 1178 | Workflow linting bypass for test code | CRITICAL:75 | **FALSE POSITIVE** | `.github/workflows/code_style.yml` | Close — script reads full file, not hunk headers | + +--- + +## Remaining Batches (queued) + +### Batch 2 — Retry/DRY + CI workflow issues (completed) + +| # | Title | Severity | Verdict | Action | +|---|-------|----------|---------|--------| +| 1288 | DRY violation: retry-after parsing | HIGH:95 | **LEGIT** | Fixed: extracted shared `parse_retry_after()` | +| 1289 | Semantic mismatch in RFC2822 test helpers | MEDIUM:85 | **DUPLICATE** (closed) | Duplicate of #1288 | +| 1290 | Unnecessary eager `chrono::Utc::now()` call | LOW:85 | **FALSE POSITIVE** (closed) | Already deferred inside successful parse branch | +| 963 | Logical equivalence bug in workflow conditions | HIGH:100 | **FALSE POSITIVE** (closed) | Refactored condition correctly handles `workflow_call` | +| 1280 | Flaky OAuth wildcard callback tests | Flaky | **LEGIT** | Fixed: added `tokio::sync::Mutex` for env var serialization | + +### Batch 3 — Routine engine + notification routing +- #1365 — too_many_arguments on RoutineEngine::new() +- #1371 — Discovery schema regeneration on every tool_info call +- #1364 — Prompt injection via unescaped channel/user in lightweight routines +- #1284 — notification_target_for_channel() assumes channel owner + +### Batch 4 — Telegram/Extension Manager webhook group +- #1247 — Synchronous 120-second blocking poll in HTTP handler +- #1248 — Hardcoded channel-specific logic violates architecture +- #1249 — Telegram-specific business logic bloats ExtensionManager +- #1250 — Response success/failure logic mismatch in chat auth +- #1251 — Channel-specific configuration mappings lack extensibility + +### Batch 5 — HMAC/Auth/Security +- #1034 — Signature verification not constant-time +- #1035 — Incorrect order of operations in HMAC verification +- #1036 — Double opt-in lacks runtime validation consistency +- #1037 — API breaking change: auth() signature +- #1038 — CSP policy allows CDN scripts with risky fallback + +### Batch 6 — Webhook handler + config +- #1039 — Per-request HTTP client creation in hot path +- #1040 — Complex nested auth logic in webhook_handler +- #1041 — Redundant JSON deserialization in webhook handler +- #1042 — Implicit state mutation in config conversion +- #1005 — Inconsistent double opt-in enforcement + +### Batch 7 — Tool schema validation / WASM bounds +- #974 — Unbounded recursion in resolve_nested() +- #975 — Unbounded recursion in validate_tool_schema() +- #976 — Unbounded description string in CapabilitiesFile +- #977 — Unbounded parameters schema JSON +- #978 — Unnecessary clone of large JSON in hot path + +### Batch 8 — Tool schema + config + security +- #979 — No size limits on JSON files read +- #980 — Misleading warning condition for missing parameters +- #988 — Hardcoded CLI_ENABLED env var in systemd template +- #990 — Configuration semantics unclear for daemon mode +- #1103 — SSRF risk via configurable embedding base URL + +### Batch 9 — Agent loop / job worker +- #870 — Unbounded loop without cancellation token +- #871 — Stringly-typed unsupported parameter filtering +- #873 — RwLock overhead on hot path +- #892 — JobDelegate::check_signals() treats non-terminal as terminal +- #1252 — String concatenation in hot polling loop + +### Batch 10 — Agent loop perf + CI scripts +- #893 — Unnecessary parameter cloning on every tool execution +- #894 — truncate_for_preview allocates for non-truncated strings +- #895 — Tool definitions fetched every iteration without caching +- #1179 — AWK state machine never resets between hunks +- #1180 — Code fence detection logic flawed in extract_suggestions() +- #1181 — Unsafe .unwrap() in production code manifest.rs diff --git a/src/llm/anthropic_oauth.rs b/src/llm/anthropic_oauth.rs index 8c701101..490fbc3f 100644 --- a/src/llm/anthropic_oauth.rs +++ b/src/llm/anthropic_oauth.rs @@ -22,8 +22,6 @@ use crate::llm::provider::{ ToolCompletionRequest, ToolCompletionResponse, strip_unsupported_completion_params, strip_unsupported_tool_params, }; -use crate::llm::retry::cap_retry_after; - const ANTHROPIC_API_URL: &str = "https://api.anthropic.com/v1/messages"; /// OAuth beta requires 2023-06-01; the 2024-10-22 version is not valid with the beta flag. const ANTHROPIC_API_VERSION: &str = "2023-06-01"; @@ -144,15 +142,9 @@ impl AnthropicOAuthProvider { if !status.is_success() { // Parse Retry-After header before consuming the body. - // Falls back to 60s if header is missing or unparseable (prevents "retry after None" errors). - let retry_after = response - .headers() - .get("retry-after") - .and_then(|v| v.to_str().ok()) - .and_then(|v| v.parse::().ok()) - .map(std::time::Duration::from_secs) - .map(cap_retry_after) - .or(Some(std::time::Duration::from_secs(60))); + let retry_after = Some(crate::llm::retry::parse_retry_after( + response.headers().get("retry-after"), + )); let response_text = response .text() @@ -709,84 +701,4 @@ mod tests { // Subsequent reads see the updated token assert_eq!(token.read().unwrap().expose_secret(), "new_token"); } - - // -- Retry-After header parsing tests (regression for rate limit "None" bug) -- - - #[test] - fn test_retry_after_parsing_delay_seconds() { - // Verify delay-seconds format is parsed correctly - let header_value = "45"; - let duration = parse_retry_after_anthropic_for_test(header_value); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(45)), - "Should parse delay-seconds format" - ); - } - - #[test] - fn test_retry_after_fallback_missing_header() { - // Regression test: When Retry-After header is missing, - // should fall back to 60s instead of None - let duration = parse_retry_after_anthropic_for_test(""); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(60)), - "Missing header should fallback to 60s" - ); - } - - #[test] - fn test_retry_after_fallback_invalid_format() { - // Regression test: When Retry-After header is in unexpected format, - // should fall back to 60s instead of None - let invalid_formats = vec![ - "invalid", - "not-a-number", - "30.5", // float instead of int - "abc123", - "Mon, 02 Mar 2026 18:00:00 GMT", // RFC2822 not supported in anthropic version - ]; - - for format in invalid_formats { - let duration = parse_retry_after_anthropic_for_test(format); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(60)), - "Invalid format '{}' should fallback to 60s", - format - ); - } - } - - #[test] - fn test_retry_after_zero_seconds_accepted() { - // Verify zero seconds is a valid retry delay - let duration = parse_retry_after_anthropic_for_test("0"); - assert_eq!(duration, Some(std::time::Duration::ZERO)); - } - - #[test] - fn test_retry_after_large_number() { - // Verify large numbers are capped to the safe maximum - let duration = parse_retry_after_anthropic_for_test("7200"); // 2 hours - assert_eq!( - duration, - Some(std::time::Duration::from_secs( - crate::llm::retry::MAX_RETRY_AFTER_SECS - )) - ); - } - - /// Helper function to test Retry-After header parsing logic for Anthropic - /// (simulates the parsing done in send_request without actual HTTP, including fallback) - fn parse_retry_after_anthropic_for_test(header_value: &str) -> Option { - header_value - .trim() - .parse::() - .ok() - .map(std::time::Duration::from_secs) - .map(cap_retry_after) - .or(Some(std::time::Duration::from_secs(60))) - } } diff --git a/src/llm/nearai_chat.rs b/src/llm/nearai_chat.rs index f0d711a9..e1a29643 100644 --- a/src/llm/nearai_chat.rs +++ b/src/llm/nearai_chat.rs @@ -22,7 +22,7 @@ use crate::llm::provider::{ ChatMessage, CompletionRequest, CompletionResponse, FinishReason, LlmProvider, Role, ToolCall, ToolCompletionRequest, ToolCompletionResponse, }; -use crate::llm::{costs, retry::cap_retry_after, session::SessionManager}; +use crate::llm::{costs, session::SessionManager}; /// Information about an available model from NEAR AI API. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -243,30 +243,9 @@ impl NearAiChatProvider { let status = response.status(); // Extract Retry-After header before consuming the response body. - // Supports both delay-seconds (RFC 7231 §7.1.3) and HTTP-date formats. - // Falls back to 60s if header is missing or unparseable (prevents "retry after None" errors). - let retry_after_header = response - .headers() - .get("retry-after") - .and_then(|v| v.to_str().ok()) - .and_then(|v| { - // Try delay-seconds first (most common from API providers) - if let Ok(secs) = v.trim().parse::() { - return Some(cap_retry_after(std::time::Duration::from_secs(secs))); - } - // Try HTTP-date (e.g. "Mon, 02 Mar 2026 18:00:00 GMT") - if let Ok(dt) = chrono::DateTime::parse_from_rfc2822(v.trim()) { - let now = chrono::Utc::now(); - let delta = dt.signed_duration_since(now); - // Use max(0) so past/present dates yield Duration::ZERO - // rather than None (which would cause an immediate retry). - return Some(cap_retry_after(std::time::Duration::from_secs( - delta.num_seconds().max(0) as u64, - ))); - } - None - }) - .or(Some(std::time::Duration::from_secs(60))); + let retry_after_header = Some(crate::llm::retry::parse_retry_after( + response.headers().get("retry-after"), + )); let response_text = response.text().await.map_err(|e| LlmError::RequestFailed { provider: "nearai_chat".to_string(), reason: format!("Failed to read response body: {}", e), @@ -2218,123 +2197,4 @@ mod tests { "http://example.com/api/proxy/v1/chat/completions" ); } - - // -- Retry-After header parsing tests (regression for rate limit "None" bug) -- - - #[test] - fn test_retry_after_parsing_delay_seconds() { - // Verify delay-seconds format (most common) is parsed correctly - let header_value = "30"; - let duration = parse_retry_after_for_test(header_value); - assert_eq!(duration, Some(std::time::Duration::from_secs(30))); - } - - #[test] - fn test_retry_after_parsing_rfc2822_date() { - // Verify HTTP-date (RFC 2822) format is parsed correctly - // Use a date 60 seconds in the future - let now = chrono::Utc::now(); - let future = now + chrono::Duration::seconds(60); - let date_str = future.to_rfc2822(); - - let duration = parse_retry_after_for_test(&date_str); - assert!(duration.is_some()); - let d = duration.unwrap(); - // Allow ±5 seconds of drift due to processing time - assert!( - d.as_secs() >= 55 && d.as_secs() <= 65, - "Expected ~60s, got {}s", - d.as_secs() - ); - } - - #[test] - fn test_retry_after_fallback_missing_header() { - // Regression test: When Retry-After header is missing, - // should fall back to 60s instead of None - let duration = parse_retry_after_for_test(""); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(60)), - "Missing header should fallback to 60s" - ); - } - - #[test] - fn test_retry_after_fallback_invalid_format() { - // Regression test: When Retry-After header is in unexpected format, - // should fall back to 60s instead of None - let invalid_formats = vec![ - "invalid", - "not-a-number", - "30.5", // float instead of int - "abc123", - ]; - - for format in invalid_formats { - let duration = parse_retry_after_for_test(format); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(60)), - "Invalid format '{}' should fallback to 60s", - format - ); - } - } - - #[test] - fn test_retry_after_past_date_returns_zero() { - // When HTTP-date is in the past, should return Duration::ZERO - // (not None, which would trigger immediate retry) - let past = chrono::Utc::now() - chrono::Duration::seconds(60); - let past_date_str = past.to_rfc2822(); - - let duration = parse_retry_after_for_test(&past_date_str); - assert_eq!( - duration, - Some(std::time::Duration::ZERO), - "Past date should return Duration::ZERO, not None" - ); - } - - #[test] - fn test_retry_after_zero_seconds_accepted() { - // Verify zero seconds is a valid retry delay - let duration = parse_retry_after_for_test("0"); - assert_eq!(duration, Some(std::time::Duration::ZERO)); - } - - #[test] - fn test_retry_after_large_number() { - // Verify large numbers are capped to the safe maximum - let duration = parse_retry_after_for_test("3600"); // 1 hour - assert_eq!(duration, Some(std::time::Duration::from_secs(3600))); - - let huge = parse_retry_after_for_test("18446744073709551615"); - assert_eq!( - huge, - Some(std::time::Duration::from_secs( - crate::llm::retry::MAX_RETRY_AFTER_SECS - )) - ); - } - - /// Helper function to test Retry-After header parsing logic - /// (simulates the parsing done in send_request without actual HTTP, including fallback) - fn parse_retry_after_for_test(header_value: &str) -> Option { - let trimmed = header_value.trim(); - let parsed = if let Ok(secs) = trimmed.parse::() { - Some(cap_retry_after(std::time::Duration::from_secs(secs))) - } else if let Ok(dt) = chrono::DateTime::parse_from_rfc2822(trimmed) { - let now = chrono::Utc::now(); - let delta = dt.signed_duration_since(now); - Some(cap_retry_after(std::time::Duration::from_secs( - delta.num_seconds().max(0) as u64, - ))) - } else { - None - }; - // Apply fallback to 60s if parsing failed (matches actual code behavior) - parsed.or(Some(std::time::Duration::from_secs(60))) - } } diff --git a/src/llm/oauth_helpers.rs b/src/llm/oauth_helpers.rs index b63457fd..2fd97c55 100644 --- a/src/llm/oauth_helpers.rs +++ b/src/llm/oauth_helpers.rs @@ -361,6 +361,7 @@ pub fn landing_html(provider_name: &str, success: bool) -> String { #[cfg(test)] mod tests { use super::*; + use crate::config::helpers::ENV_MUTEX; #[test] fn loopback_detection() { @@ -385,12 +386,22 @@ mod tests { assert!(!is_wildcard_host("localhost")); } + // Lock held across await to serialize env-var mutation; the awaited op is a quick local TCP bind. + #[allow(clippy::await_holding_lock)] #[tokio::test] async fn bind_rejects_wildcard_ipv4() { - // SAFETY: test is single-threaded; env var is restored immediately after. + let _guard = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let original = std::env::var("OAUTH_CALLBACK_HOST").ok(); + // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { std::env::set_var("OAUTH_CALLBACK_HOST", "0.0.0.0") }; let result = bind_callback_listener().await; - unsafe { std::env::remove_var("OAUTH_CALLBACK_HOST") }; + // SAFETY: Under ENV_MUTEX, no concurrent env access. + unsafe { + match &original { + Some(v) => std::env::set_var("OAUTH_CALLBACK_HOST", v), + None => std::env::remove_var("OAUTH_CALLBACK_HOST"), + } + } assert!(result.is_err()); let err = result.unwrap_err().to_string(); assert!( @@ -399,12 +410,22 @@ mod tests { ); } + // Lock held across await to serialize env-var mutation; the awaited op is a quick local TCP bind. + #[allow(clippy::await_holding_lock)] #[tokio::test] async fn bind_rejects_wildcard_ipv6() { - // SAFETY: test is single-threaded; env var is restored immediately after. + let _guard = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let original = std::env::var("OAUTH_CALLBACK_HOST").ok(); + // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { std::env::set_var("OAUTH_CALLBACK_HOST", "::") }; let result = bind_callback_listener().await; - unsafe { std::env::remove_var("OAUTH_CALLBACK_HOST") }; + // SAFETY: Under ENV_MUTEX, no concurrent env access. + unsafe { + match &original { + Some(v) => std::env::set_var("OAUTH_CALLBACK_HOST", v), + None => std::env::remove_var("OAUTH_CALLBACK_HOST"), + } + } assert!(result.is_err()); let err = result.unwrap_err().to_string(); assert!( diff --git a/src/llm/retry.rs b/src/llm/retry.rs index 6250de33..78a26b27 100644 --- a/src/llm/retry.rs +++ b/src/llm/retry.rs @@ -78,6 +78,33 @@ pub(crate) fn cap_retry_after(duration: Duration) -> Duration { duration.min(Duration::from_secs(MAX_RETRY_AFTER_SECS)) } +/// Parse a `Retry-After` header value into a capped `Duration`. +/// +/// Supports both delay-seconds (RFC 7231 §7.1.3) and HTTP-date formats (RFC 7231 +/// §7.1.1 / IMF-fixdate). The implementation uses `chrono::DateTime::parse_from_rfc2822`, +/// which also accepts RFC 2822-style dates. +/// Returns `DEFAULT_RETRY_AFTER` (60 s) if the header is missing or unparseable. +pub(crate) fn parse_retry_after(header: Option<&reqwest::header::HeaderValue>) -> Duration { + header + .and_then(|v| v.to_str().ok()) + .and_then(|v| { + if let Ok(secs) = v.trim().parse::() { + return Some(cap_retry_after(Duration::from_secs(secs))); + } + if let Ok(dt) = chrono::DateTime::parse_from_rfc2822(v.trim()) { + let now = chrono::Utc::now(); + let delta = dt.signed_duration_since(now); + return Some(cap_retry_after(Duration::from_secs( + delta.num_seconds().max(0) as u64, + ))); + } + None + }) + .unwrap_or(Duration::from_secs(DEFAULT_RETRY_AFTER_SECS)) +} + +const DEFAULT_RETRY_AFTER_SECS: u64 = 60; + /// Configuration for the retry decorator. #[derive(Debug, Clone)] pub struct RetryConfig { @@ -444,4 +471,53 @@ mod tests { Duration::from_secs(0) ); } + + #[test] + fn parse_retry_after_delay_seconds() { + let val = reqwest::header::HeaderValue::from_static("30"); + assert_eq!(parse_retry_after(Some(&val)), Duration::from_secs(30)); + } + + #[test] + fn parse_retry_after_missing_header() { + assert_eq!( + parse_retry_after(None), + Duration::from_secs(DEFAULT_RETRY_AFTER_SECS) + ); + } + + #[test] + fn parse_retry_after_unparseable() { + let val = reqwest::header::HeaderValue::from_static("not-a-number"); + assert_eq!( + parse_retry_after(Some(&val)), + Duration::from_secs(DEFAULT_RETRY_AFTER_SECS) + ); + } + + #[test] + fn parse_retry_after_clamps_large_value() { + let val = reqwest::header::HeaderValue::from_static("999999"); + assert_eq!( + parse_retry_after(Some(&val)), + Duration::from_secs(MAX_RETRY_AFTER_SECS) + ); + } + + #[test] + fn parse_retry_after_http_date() { + let future = chrono::Utc::now() + chrono::Duration::seconds(30); + let date_str = future.to_rfc2822(); + let val = reqwest::header::HeaderValue::from_str(&date_str).unwrap(); + let parsed = parse_retry_after(Some(&val)); + let diff = if parsed > Duration::from_secs(30) { + parsed - Duration::from_secs(30) + } else { + Duration::from_secs(30) - parsed + }; + assert!( + diff <= Duration::from_secs(2), + "expected ~30s, got {parsed:?} (diff {diff:?}) from header {date_str:?}" + ); + } } diff --git a/src/workspace/embeddings.rs b/src/workspace/embeddings.rs index a8ed0a3e..99a3a850 100644 --- a/src/workspace/embeddings.rs +++ b/src/workspace/embeddings.rs @@ -6,8 +6,6 @@ use async_trait::async_trait; use serde::{Deserialize, Serialize}; -use crate::llm::retry::cap_retry_after; - /// Error type for embedding operations. #[derive(Debug, thiserror::Error)] pub enum EmbeddingError { @@ -228,14 +226,9 @@ impl EmbeddingProvider for OpenAiEmbeddings { } if status == reqwest::StatusCode::TOO_MANY_REQUESTS { - let retry_after = response - .headers() - .get("retry-after") - .and_then(|v| v.to_str().ok()) - .and_then(|s| s.parse::().ok()) - .map(std::time::Duration::from_secs) - .map(cap_retry_after) - .or(Some(std::time::Duration::from_secs(60))); + let retry_after = Some(crate::llm::retry::parse_retry_after( + response.headers().get("retry-after"), + )); return Err(EmbeddingError::RateLimited { retry_after }); } @@ -371,14 +364,9 @@ impl EmbeddingProvider for NearAiEmbeddings { } if status == reqwest::StatusCode::TOO_MANY_REQUESTS { - let retry_after = response - .headers() - .get("retry-after") - .and_then(|v| v.to_str().ok()) - .and_then(|s| s.parse::().ok()) - .map(std::time::Duration::from_secs) - .map(cap_retry_after) - .or(Some(std::time::Duration::from_secs(60))); + let retry_after = Some(crate::llm::retry::parse_retry_after( + response.headers().get("retry-after"), + )); return Err(EmbeddingError::RateLimited { retry_after }); } @@ -652,49 +640,4 @@ mod tests { let provider = OpenAiEmbeddings::new("test-key").with_base_url("custom.example.com/v1"); assert_eq!(provider.base_url, "https://custom.example.com/v1"); } - - // -- Retry-After header parsing tests (regression for rate limit "None" bug) -- - - #[test] - fn test_retry_after_parsing_delay_seconds() { - // Verify delay-seconds format is parsed correctly - let header_value = "120"; - let duration = parse_retry_after_embeddings_for_test(header_value); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(120)), - "Should parse delay-seconds format" - ); - } - - #[test] - fn test_retry_after_fallback_missing_header() { - // Regression test: When Retry-After header is missing, - // should fall back to 60s instead of None - let duration = parse_retry_after_embeddings_for_test(""); - assert_eq!( - duration, - Some(std::time::Duration::from_secs(60)), - "Missing header should fallback to 60s" - ); - } - - #[test] - fn test_retry_after_zero_seconds_accepted() { - // Verify zero seconds is a valid retry delay - let duration = parse_retry_after_embeddings_for_test("0"); - assert_eq!(duration, Some(std::time::Duration::ZERO)); - } - - /// Helper function to test Retry-After header parsing logic for embeddings - /// (simulates the parsing done in embed without actual HTTP, including fallback) - fn parse_retry_after_embeddings_for_test(header_value: &str) -> Option { - header_value - .trim() - .parse::() - .ok() - .map(std::time::Duration::from_secs) - .map(cap_retry_after) - .or(Some(std::time::Duration::from_secs(60))) - } }