From d8c716f99aa7b388a08672ff03b1bcd3d12b3b39 Mon Sep 17 00:00:00 2001 From: argenis de la rosa Date: Thu, 5 Mar 2026 02:13:43 -0500 Subject: [PATCH] fix(cron,security,onboarding): unify shell policy and custom-home-safe persistence (replay #2797) --- src/config/schema.rs | 182 ++++++++++++++++++++++++++++++--------- src/cron/mod.rs | 157 ++++++++++++++++++++++++++++++--- src/cron/scheduler.rs | 103 +++++++++++++++------- src/gateway/api.rs | 8 +- src/onboard/wizard.rs | 16 ++-- src/tools/cron_add.rs | 10 +-- src/tools/cron_remove.rs | 4 +- src/tools/cron_run.rs | 18 +++- src/tools/cron_update.rs | 16 +--- 9 files changed, 396 insertions(+), 118 deletions(-) diff --git a/src/config/schema.rs b/src/config/schema.rs index 0a9100e4d..d4b3c6a2a 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -6611,8 +6611,8 @@ fn default_config_dir() -> Result { Ok(home.join(".zeroclaw")) } -fn active_workspace_state_path(default_dir: &Path) -> PathBuf { - default_dir.join(ACTIVE_WORKSPACE_STATE_FILE) +fn active_workspace_state_path(marker_root: &Path) -> PathBuf { + marker_root.join(ACTIVE_WORKSPACE_STATE_FILE) } /// Returns `true` if `path` lives under the OS temp directory. @@ -6727,9 +6727,65 @@ async fn load_persisted_workspace_dirs( Ok(Some((config_dir.clone(), config_dir.join("workspace")))) } +async fn remove_active_workspace_marker(marker_root: &Path) -> Result<()> { + let state_path = active_workspace_state_path(marker_root); + if !state_path.exists() { + return Ok(()); + } + + fs::remove_file(&state_path).await.with_context(|| { + format!( + "Failed to clear active workspace marker: {}", + state_path.display() + ) + })?; + + if marker_root.exists() { + sync_directory(marker_root).await?; + } + Ok(()) +} + +async fn write_active_workspace_marker(marker_root: &Path, config_dir: &Path) -> Result<()> { + fs::create_dir_all(marker_root).await.with_context(|| { + format!( + "Failed to create active workspace marker root: {}", + marker_root.display() + ) + })?; + + let state = ActiveWorkspaceState { + config_dir: config_dir.to_string_lossy().into_owned(), + }; + let serialized = + toml::to_string_pretty(&state).context("Failed to serialize active workspace marker")?; + + let temp_path = marker_root.join(format!( + ".{ACTIVE_WORKSPACE_STATE_FILE}.tmp-{}", + uuid::Uuid::new_v4() + )); + fs::write(&temp_path, serialized).await.with_context(|| { + format!( + "Failed to write temporary active workspace marker: {}", + temp_path.display() + ) + })?; + + let state_path = active_workspace_state_path(marker_root); + if let Err(error) = fs::rename(&temp_path, &state_path).await { + let _ = fs::remove_file(&temp_path).await; + anyhow::bail!( + "Failed to atomically persist active workspace marker {}: {error}", + state_path.display() + ); + } + + sync_directory(marker_root).await?; + Ok(()) +} + pub(crate) async fn persist_active_workspace_config_dir(config_dir: &Path) -> Result<()> { let default_config_dir = default_config_dir()?; - let state_path = active_workspace_state_path(&default_config_dir); // Guard: never persist a temp-directory path as the active workspace. // This prevents transient test runs or one-off invocations from hijacking @@ -6744,48 +6800,21 @@ pub(crate) async fn persist_active_workspace_config_dir(config_dir: &Path) -> Re } if config_dir == default_config_dir { - if state_path.exists() { - fs::remove_file(&state_path).await.with_context(|| { - format!( - "Failed to clear active workspace marker: {}", - state_path.display() - ) - })?; - } + remove_active_workspace_marker(&default_config_dir).await?; return Ok(()); } - fs::create_dir_all(&default_config_dir) - .await - .with_context(|| { - format!( - "Failed to create default config directory: {}", - default_config_dir.display() - ) - })?; + // Primary marker lives with the selected config root to keep custom-home + // layouts self-contained and writable in restricted environments. + write_active_workspace_marker(config_dir, config_dir).await?; - let state = ActiveWorkspaceState { - config_dir: config_dir.to_string_lossy().into_owned(), - }; - let serialized = - toml::to_string_pretty(&state).context("Failed to serialize active workspace marker")?; - - let temp_path = default_config_dir.join(format!( - ".{ACTIVE_WORKSPACE_STATE_FILE}.tmp-{}", - uuid::Uuid::new_v4() - )); - fs::write(&temp_path, serialized).await.with_context(|| { - format!( - "Failed to write temporary active workspace marker: {}", - temp_path.display() - ) - })?; - - if let Err(error) = fs::rename(&temp_path, &state_path).await { - let _ = fs::remove_file(&temp_path).await; - anyhow::bail!( - "Failed to atomically persist active workspace marker {}: {error}", - state_path.display() + // Mirror into the default HOME-scoped root as a best-effort pointer for + // later auto-discovery. Failure here must not break onboarding/update flows. + if let Err(error) = write_active_workspace_marker(&default_config_dir, config_dir).await { + tracing::warn!( + selected_config_dir = %config_dir.display(), + default_config_dir = %default_config_dir.display(), + "Failed to mirror active workspace marker to default HOME config root; continuing with selected-root marker only: {error}" ); } @@ -13584,6 +13613,74 @@ default_model = "legacy-model" let _ = fs::remove_dir_all(temp_home).await; } + #[test] + async fn persist_active_workspace_marker_is_written_to_selected_config_root() { + let _env_guard = env_override_lock().await; + let temp_home = + std::env::temp_dir().join(format!("zeroclaw_test_home_{}", uuid::Uuid::new_v4())); + let default_config_dir = temp_home.join(".zeroclaw"); + let custom_config_dir = temp_home.join("profiles").join("custom-profile"); + let default_marker_path = default_config_dir.join(ACTIVE_WORKSPACE_STATE_FILE); + let custom_marker_path = custom_config_dir.join(ACTIVE_WORKSPACE_STATE_FILE); + + let original_home = std::env::var("HOME").ok(); + std::env::set_var("HOME", &temp_home); + + persist_active_workspace_config_dir(&custom_config_dir) + .await + .unwrap(); + + assert!(custom_marker_path.exists()); + assert!(default_marker_path.exists()); + + let custom_state: ActiveWorkspaceState = + toml::from_str(&fs::read_to_string(&custom_marker_path).await.unwrap()).unwrap(); + assert_eq!(PathBuf::from(custom_state.config_dir), custom_config_dir); + + let default_state: ActiveWorkspaceState = + toml::from_str(&fs::read_to_string(&default_marker_path).await.unwrap()).unwrap(); + assert_eq!(PathBuf::from(default_state.config_dir), custom_config_dir); + + if let Some(home) = original_home { + std::env::set_var("HOME", home); + } else { + std::env::remove_var("HOME"); + } + let _ = fs::remove_dir_all(temp_home).await; + } + + #[test] + async fn persist_active_workspace_marker_tolerates_restricted_default_home_root() { + let _env_guard = env_override_lock().await; + let temp_home = + std::env::temp_dir().join(format!("zeroclaw_test_home_{}", uuid::Uuid::new_v4())); + let default_config_root_blocker = temp_home.join(".zeroclaw"); + let custom_config_dir = temp_home.join("profiles").join("restricted-home-profile"); + let custom_marker_path = custom_config_dir.join(ACTIVE_WORKSPACE_STATE_FILE); + + fs::create_dir_all(&custom_config_dir).await.unwrap(); + fs::write(&default_config_root_blocker, "blocked-as-file") + .await + .unwrap(); + + let original_home = std::env::var("HOME").ok(); + std::env::set_var("HOME", &temp_home); + + persist_active_workspace_config_dir(&custom_config_dir) + .await + .unwrap(); + + assert!(custom_marker_path.exists()); + assert!(default_config_root_blocker.is_file()); + + if let Some(home) = original_home { + std::env::set_var("HOME", home); + } else { + std::env::remove_var("HOME"); + } + let _ = fs::remove_dir_all(temp_home).await; + } + #[test] async fn persist_active_workspace_marker_is_cleared_for_default_config_dir() { let _env_guard = env_override_lock().await; @@ -13592,6 +13689,7 @@ default_model = "legacy-model" let default_config_dir = temp_home.join(".zeroclaw"); let custom_config_dir = temp_home.join("profiles").join("custom-profile"); let marker_path = default_config_dir.join(ACTIVE_WORKSPACE_STATE_FILE); + let custom_marker_path = custom_config_dir.join(ACTIVE_WORKSPACE_STATE_FILE); let original_home = std::env::var("HOME").ok(); std::env::set_var("HOME", &temp_home); @@ -13600,11 +13698,13 @@ default_model = "legacy-model" .await .unwrap(); assert!(marker_path.exists()); + assert!(custom_marker_path.exists()); persist_active_workspace_config_dir(&default_config_dir) .await .unwrap(); assert!(!marker_path.exists()); + assert!(custom_marker_path.exists()); if let Some(home) = original_home { std::env::set_var("HOME", home); diff --git a/src/cron/mod.rs b/src/cron/mod.rs index d93ce6d98..babbde82b 100644 --- a/src/cron/mod.rs +++ b/src/cron/mod.rs @@ -1,6 +1,6 @@ use crate::config::Config; use crate::security::SecurityPolicy; -use anyhow::{bail, Result}; +use anyhow::{anyhow, bail, Result}; pub mod consolidation; mod schedule; @@ -15,11 +15,63 @@ pub use schedule::{ }; #[allow(unused_imports)] pub use store::{ - add_agent_job, add_job, add_shell_job, due_jobs, get_job, list_jobs, list_runs, - record_last_run, record_run, remove_job, reschedule_after_run, update_job, + add_agent_job, due_jobs, get_job, list_jobs, list_runs, record_last_run, record_run, + remove_job, reschedule_after_run, update_job, }; pub use types::{CronJob, CronJobPatch, CronRun, DeliveryConfig, JobType, Schedule, SessionTarget}; +fn validate_shell_command(config: &Config, command: &str, approved: bool) -> Result<()> { + let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); + security + .validate_command_execution(command, approved) + .map(|_| ()) + .map_err(|reason| anyhow!("Command blocked by security policy: {reason}")) +} + +pub fn add_shell_job( + config: &Config, + name: Option, + schedule: Schedule, + command: &str, +) -> Result { + add_shell_job_with_approval(config, name, schedule, command, false) +} + +pub fn add_shell_job_with_approval( + config: &Config, + name: Option, + schedule: Schedule, + command: &str, + approved: bool, +) -> Result { + validate_shell_command(config, command, approved)?; + store::add_shell_job(config, name, schedule, command) +} + +pub fn add_job(config: &Config, expression: &str, command: &str) -> Result { + let schedule = Schedule::Cron { + expr: expression.to_string(), + tz: None, + }; + add_shell_job(config, None, schedule, command) +} + +pub fn update_shell_job(config: &Config, job_id: &str, patch: CronJobPatch) -> Result { + update_shell_job_with_approval(config, job_id, patch, false) +} + +pub fn update_shell_job_with_approval( + config: &Config, + job_id: &str, + patch: CronJobPatch, + approved: bool, +) -> Result { + if let Some(command) = patch.command.as_deref() { + validate_shell_command(config, command, approved)?; + } + update_job(config, job_id, patch) +} + #[allow(clippy::needless_pass_by_value)] pub fn handle_command(command: crate::CronCommands, config: &Config) -> Result<()> { match command { @@ -129,13 +181,6 @@ pub fn handle_command(command: crate::CronCommands, config: &Config) -> Result<( None }; - if let Some(ref cmd) = command { - let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); - if !security.is_command_allowed(cmd) { - bail!("Command blocked by security policy: {cmd}"); - } - } - let patch = CronJobPatch { schedule, command, @@ -143,7 +188,7 @@ pub fn handle_command(command: crate::CronCommands, config: &Config) -> Result<( ..CronJobPatch::default() }; - let job = update_job(config, &id, patch)?; + let job = update_shell_job(config, &id, patch)?; println!("\u{2705} Updated cron job {}", job.id); println!(" Expr: {}", job.expression); println!(" Next: {}", job.next_run.to_rfc3339()); @@ -414,4 +459,94 @@ mod tests { let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); assert!(security.is_command_allowed("echo safe")); } + + #[test] + fn add_shell_job_requires_explicit_approval_for_medium_risk() { + let tmp = TempDir::new().unwrap(); + let mut config = test_config(&tmp); + config.autonomy.allowed_commands = vec!["echo".into(), "touch".into()]; + + let denied = add_shell_job( + &config, + None, + Schedule::Cron { + expr: "*/5 * * * *".into(), + tz: None, + }, + "touch cron-medium-risk", + ); + assert!(denied.is_err()); + assert!(denied + .unwrap_err() + .to_string() + .contains("explicit approval")); + + let approved = add_shell_job_with_approval( + &config, + None, + Schedule::Cron { + expr: "*/5 * * * *".into(), + tz: None, + }, + "touch cron-medium-risk", + true, + ); + assert!(approved.is_ok(), "{approved:?}"); + } + + #[test] + fn update_requires_explicit_approval_for_medium_risk() { + let tmp = TempDir::new().unwrap(); + let mut config = test_config(&tmp); + config.autonomy.allowed_commands = vec!["echo".into(), "touch".into()]; + let job = make_job(&config, "*/5 * * * *", None, "echo original"); + + let denied = update_shell_job( + &config, + &job.id, + CronJobPatch { + command: Some("touch cron-medium-risk-update".into()), + ..CronJobPatch::default() + }, + ); + assert!(denied.is_err()); + assert!(denied + .unwrap_err() + .to_string() + .contains("explicit approval")); + + let approved = update_shell_job_with_approval( + &config, + &job.id, + CronJobPatch { + command: Some("touch cron-medium-risk-update".into()), + ..CronJobPatch::default() + }, + true, + ) + .unwrap(); + assert_eq!(approved.command, "touch cron-medium-risk-update"); + } + + #[test] + fn cli_update_requires_explicit_approval_for_medium_risk() { + let tmp = TempDir::new().unwrap(); + let mut config = test_config(&tmp); + config.autonomy.allowed_commands = vec!["echo".into(), "touch".into()]; + let job = make_job(&config, "*/5 * * * *", None, "echo original"); + + let result = run_update( + &config, + &job.id, + None, + None, + Some("touch cron-cli-medium-risk"), + None, + ); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("explicit approval")); + } } diff --git a/src/cron/scheduler.rs b/src/cron/scheduler.rs index ea46e5f6b..ea0813fe7 100644 --- a/src/cron/scheduler.rs +++ b/src/cron/scheduler.rs @@ -58,14 +58,23 @@ pub async fn run(config: Config) -> Result<()> { } pub async fn execute_job_now(config: &Config, job: &CronJob) -> (bool, String) { + execute_job_now_with_approval(config, job, false).await +} + +pub async fn execute_job_now_with_approval( + config: &Config, + job: &CronJob, + approved: bool, +) -> (bool, String) { let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); - Box::pin(execute_job_with_retry(config, &security, job)).await + execute_job_with_retry(config, &security, job, approved).await } async fn execute_job_with_retry( config: &Config, security: &SecurityPolicy, job: &CronJob, + approved: bool, ) -> (bool, String) { let mut last_output = String::new(); let retries = config.reliability.scheduler_retries; @@ -73,8 +82,8 @@ async fn execute_job_with_retry( for attempt in 0..=retries { let (success, output) = match job.job_type { - JobType::Shell => run_job_command(config, security, job).await, - JobType::Agent => Box::pin(run_agent_job(config, security, job)).await, + JobType::Shell => run_job_command_with_approval(config, security, job, approved).await, + JobType::Agent => run_agent_job(config, security, job).await, }; last_output = output; @@ -140,7 +149,7 @@ async fn execute_and_persist_job( warn_if_high_frequency_agent_job(job); let started_at = Utc::now(); - let (success, output) = Box::pin(execute_job_with_retry(config, security, job)).await; + let (success, output) = execute_job_with_retry(config, security, job, false).await; let finished_at = Utc::now(); let success = persist_job_result(config, job, success, &output, started_at, finished_at).await; @@ -539,12 +548,22 @@ async fn run_job_command( config: &Config, security: &SecurityPolicy, job: &CronJob, +) -> (bool, String) { + run_job_command_with_approval(config, security, job, false).await +} + +async fn run_job_command_with_approval( + config: &Config, + security: &SecurityPolicy, + job: &CronJob, + approved: bool, ) -> (bool, String) { run_job_command_with_timeout( config, security, job, Duration::from_secs(SHELL_JOB_TIMEOUT_SECS), + approved, ) .await } @@ -554,6 +573,7 @@ async fn run_job_command_with_timeout( security: &SecurityPolicy, job: &CronJob, timeout: Duration, + approved: bool, ) -> (bool, String) { if !security.can_act() { return ( @@ -569,21 +589,8 @@ async fn run_job_command_with_timeout( ); } - if !security.is_command_allowed(&job.command) { - return ( - false, - format!( - "blocked by security policy: command not allowed: {}", - job.command - ), - ); - } - - if let Some(path) = security.forbidden_path_argument(&job.command) { - return ( - false, - format!("blocked by security policy: forbidden path argument: {path}"), - ); + if let Err(reason) = security.validate_command_execution(&job.command, approved) { + return (false, format!("blocked by security policy: {reason}")); } if !security.record_action() { @@ -769,8 +776,14 @@ mod tests { let job = test_job("sleep 1"); let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); - let (success, output) = - run_job_command_with_timeout(&config, &security, &job, Duration::from_millis(50)).await; + let (success, output) = run_job_command_with_timeout( + &config, + &security, + &job, + Duration::from_millis(50), + false, + ) + .await; assert!(!success); assert!(output.contains("job timed out after")); } @@ -786,7 +799,7 @@ mod tests { let (success, output) = run_job_command(&config, &security, &job).await; assert!(!success); assert!(output.contains("blocked by security policy")); - assert!(output.contains("command not allowed")); + assert!(output.contains("Command not allowed")); } #[tokio::test] @@ -800,7 +813,7 @@ mod tests { let (success, output) = run_job_command(&config, &security, &job).await; assert!(!success); assert!(output.contains("blocked by security policy")); - assert!(output.contains("forbidden path argument")); + assert!(output.contains("Path blocked by security policy")); assert!(output.contains("/etc/passwd")); } @@ -815,7 +828,7 @@ mod tests { let (success, output) = run_job_command(&config, &security, &job).await; assert!(!success); assert!(output.contains("blocked by security policy")); - assert!(output.contains("forbidden path argument")); + assert!(output.contains("Path blocked by security policy")); assert!(output.contains("/etc/passwd")); } @@ -830,7 +843,7 @@ mod tests { let (success, output) = run_job_command(&config, &security, &job).await; assert!(!success); assert!(output.contains("blocked by security policy")); - assert!(output.contains("forbidden path argument")); + assert!(output.contains("Path blocked by security policy")); assert!(output.contains("/etc/passwd")); } @@ -845,7 +858,7 @@ mod tests { let (success, output) = run_job_command(&config, &security, &job).await; assert!(!success); assert!(output.contains("blocked by security policy")); - assert!(output.contains("forbidden path argument")); + assert!(output.contains("Path blocked by security policy")); assert!(output.contains("~root/.ssh/id_rsa")); } @@ -860,7 +873,39 @@ mod tests { let (success, output) = run_job_command(&config, &security, &job).await; assert!(!success); assert!(output.contains("blocked by security policy")); - assert!(output.contains("command not allowed")); + assert!(output.contains("Command not allowed")); + } + + #[tokio::test] + async fn run_job_command_blocks_medium_risk_without_explicit_approval() { + let tmp = TempDir::new().unwrap(); + let mut config = test_config(&tmp).await; + config.autonomy.allowed_commands = vec!["touch".into()]; + let job = test_job("touch cron-scheduler-approval-needed"); + let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); + + let (success, output) = run_job_command(&config, &security, &job).await; + assert!(!success); + assert!(output.contains("blocked by security policy")); + assert!(output.contains("explicit approval")); + } + + #[tokio::test] + async fn execute_job_now_with_approval_allows_medium_risk_shell_command() { + let tmp = TempDir::new().unwrap(); + let mut config = test_config(&tmp).await; + config.autonomy.allowed_commands = vec!["touch".into()]; + let marker = "scheduler-approved-marker"; + let marker_path = config.workspace_dir.join(marker); + let job = test_job(&format!("touch {marker}")); + + let (denied, denied_output) = execute_job_now(&config, &job).await; + assert!(!denied); + assert!(denied_output.contains("explicit approval")); + + let (approved, output) = execute_job_now_with_approval(&config, &job, true).await; + assert!(approved, "{output}"); + assert!(marker_path.exists()); } #[tokio::test] @@ -908,7 +953,7 @@ mod tests { .unwrap(); let job = test_job("sh ./retry-once.sh"); - let (success, output) = execute_job_with_retry(&config, &security, &job).await; + let (success, output) = execute_job_with_retry(&config, &security, &job, false).await; assert!(success); assert!(output.contains("recovered")); } @@ -923,7 +968,7 @@ mod tests { let job = test_job("ls always_missing_for_retry_test"); - let (success, output) = execute_job_with_retry(&config, &security, &job).await; + let (success, output) = execute_job_with_retry(&config, &security, &job, false).await; assert!(!success); assert!(output.contains("always_missing_for_retry_test")); } diff --git a/src/gateway/api.rs b/src/gateway/api.rs index 01aef8cac..76aebe4b6 100644 --- a/src/gateway/api.rs +++ b/src/gateway/api.rs @@ -286,7 +286,13 @@ pub async fn handle_api_cron_add( tz: None, }; - match crate::cron::add_shell_job(&config, body.name, schedule, &body.command) { + match crate::cron::add_shell_job_with_approval( + &config, + body.name, + schedule, + &body.command, + false, + ) { Ok(job) => Json(serde_json::json!({ "status": "ok", "job": { diff --git a/src/onboard/wizard.rs b/src/onboard/wizard.rs index a48bf06a2..4314154ac 100644 --- a/src/onboard/wizard.rs +++ b/src/onboard/wizard.rs @@ -2541,14 +2541,14 @@ async fn persist_workspace_selection(config_path: &Path) -> Result<()> { let config_dir = config_path .parent() .context("Config path must have a parent directory")?; - crate::config::schema::persist_active_workspace_config_dir(config_dir) - .await - .with_context(|| { - format!( - "Failed to persist active workspace selection for {}", - config_dir.display() - ) - }) + if let Err(error) = crate::config::schema::persist_active_workspace_config_dir(config_dir).await + { + tracing::warn!( + config_dir = %config_dir.display(), + "Could not persist active workspace marker; continuing without marker: {error}" + ); + } + Ok(()) } // ── Step 1: Workspace ──────────────────────────────────────────── diff --git a/src/tools/cron_add.rs b/src/tools/cron_add.rs index f77fc02a1..935d71755 100644 --- a/src/tools/cron_add.rs +++ b/src/tools/cron_add.rs @@ -184,19 +184,11 @@ impl Tool for CronAddTool { } }; - if let Err(reason) = self.security.validate_command_execution(command, approved) { - return Ok(ToolResult { - success: false, - output: String::new(), - error: Some(reason), - }); - } - if let Some(blocked) = self.enforce_mutation_allowed("cron_add") { return Ok(blocked); } - cron::add_shell_job(&self.config, name, schedule, command) + cron::add_shell_job_with_approval(&self.config, name, schedule, command, approved) } JobType::Agent => { let prompt = match args.get("prompt").and_then(serde_json::Value::as_str) { diff --git a/src/tools/cron_remove.rs b/src/tools/cron_remove.rs index b4dc110c9..e74bcb143 100644 --- a/src/tools/cron_remove.rs +++ b/src/tools/cron_remove.rs @@ -166,10 +166,10 @@ mod tests { config_path: tmp.path().join("config.toml"), ..Config::default() }; - config.autonomy.level = AutonomyLevel::ReadOnly; std::fs::create_dir_all(&config.workspace_dir).unwrap(); + let job = cron::add_job(&config, "*/5 * * * *", "echo ok").unwrap(); + config.autonomy.level = AutonomyLevel::ReadOnly; let cfg = Arc::new(config); - let job = cron::add_job(&cfg, "*/5 * * * *", "echo ok").unwrap(); let tool = CronRemoveTool::new(cfg.clone(), test_security(&cfg)); let result = tool.execute(json!({"job_id": job.id})).await.unwrap(); diff --git a/src/tools/cron_run.rs b/src/tools/cron_run.rs index 2d73f414d..eb6a3b5e6 100644 --- a/src/tools/cron_run.rs +++ b/src/tools/cron_run.rs @@ -117,7 +117,7 @@ impl Tool for CronRunTool { let started_at = Utc::now(); let (success, output) = - Box::pin(cron::scheduler::execute_job_now(&self.config, &job)).await; + cron::scheduler::execute_job_now_with_approval(&self.config, &job, approved).await; let finished_at = Utc::now(); let duration_ms = (finished_at - started_at).num_milliseconds(); let status = if success { "ok" } else { "error" }; @@ -212,10 +212,10 @@ mod tests { config_path: tmp.path().join("config.toml"), ..Config::default() }; - config.autonomy.level = AutonomyLevel::ReadOnly; std::fs::create_dir_all(&config.workspace_dir).unwrap(); + let job = cron::add_job(&config, "*/5 * * * *", "echo run-now").unwrap(); + config.autonomy.level = AutonomyLevel::ReadOnly; let cfg = Arc::new(config); - let job = cron::add_job(&cfg, "*/5 * * * *", "echo run-now").unwrap(); let tool = CronRunTool::new(cfg.clone(), test_security(&cfg)); let result = tool.execute(json!({ "job_id": job.id })).await.unwrap(); @@ -234,8 +234,18 @@ mod tests { config.autonomy.level = AutonomyLevel::Supervised; config.autonomy.allowed_commands = vec!["touch".into()]; std::fs::create_dir_all(&config.workspace_dir).unwrap(); + let job = cron::add_shell_job_with_approval( + &config, + None, + crate::cron::Schedule::Cron { + expr: "*/5 * * * *".into(), + tz: None, + }, + "touch cron-run-approval", + true, + ) + .unwrap(); let cfg = Arc::new(config); - let job = cron::add_job(&cfg, "*/5 * * * *", "touch cron-run-approval").unwrap(); let tool = CronRunTool::new(cfg.clone(), test_security(&cfg)); let denied = tool.execute(json!({ "job_id": job.id })).await.unwrap(); diff --git a/src/tools/cron_update.rs b/src/tools/cron_update.rs index f41bacb15..9f3457b76 100644 --- a/src/tools/cron_update.rs +++ b/src/tools/cron_update.rs @@ -119,21 +119,11 @@ impl Tool for CronUpdateTool { .and_then(serde_json::Value::as_bool) .unwrap_or(false); - if let Some(command) = &patch.command { - if let Err(reason) = self.security.validate_command_execution(command, approved) { - return Ok(ToolResult { - success: false, - output: String::new(), - error: Some(reason), - }); - } - } - if let Some(blocked) = self.enforce_mutation_allowed("cron_update") { return Ok(blocked); } - match cron::update_job(&self.config, job_id, patch) { + match cron::update_shell_job_with_approval(&self.config, job_id, patch, approved) { Ok(job) => Ok(ToolResult { success: true, output: serde_json::to_string_pretty(&job)?, @@ -228,10 +218,10 @@ mod tests { config_path: tmp.path().join("config.toml"), ..Config::default() }; - config.autonomy.level = AutonomyLevel::ReadOnly; std::fs::create_dir_all(&config.workspace_dir).unwrap(); + let job = cron::add_job(&config, "*/5 * * * *", "echo ok").unwrap(); + config.autonomy.level = AutonomyLevel::ReadOnly; let cfg = Arc::new(config); - let job = cron::add_job(&cfg, "*/5 * * * *", "echo ok").unwrap(); let tool = CronUpdateTool::new(cfg.clone(), test_security(&cfg)); let result = tool