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/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/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/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/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() 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))) - } } 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; + } }