Merge pull request #2812 from zeroclaw-labs/replay/pr-2797-main-20260305

fix(cron,security,onboarding): unify shell policy and custom-home-safe persistence

src/config/schema.rs

@@ -6611,8 +6611,8 @@ fn default_config_dir() -> Result<PathBuf> {
     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);

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<String>,
+    schedule: Schedule,
+    command: &str,
+) -> Result<CronJob> {
+    add_shell_job_with_approval(config, name, schedule, command, false)
+}
+
+pub fn add_shell_job_with_approval(
+    config: &Config,
+    name: Option<String>,
+    schedule: Schedule,
+    command: &str,
+    approved: bool,
+) -> Result<CronJob> {
+    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<CronJob> {
+    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<CronJob> {
+    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<CronJob> {
+    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"));
+    }
 }

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"));
     }

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": {

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 ────────────────────────────────────────────

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) {

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();

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();

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