feat(cron): add get and update actions to cron tool

Add GetJob and improved UpdateJob to CronService with proper cloning, schedule diffing, and next-run recomputation. Expose get/update actions in the cron tool so agents can inspect and partially update jobs without losing payloads or needing remove+add cycles. Includes access control for remote channels and command safety gates.
2026-06-12 18:08:54 +00:00 · 2026-05-31 01:47:13 +08:00
parent e81d37108b
commit 1d8ef7dcfb
5 changed files with 767 additions and 7 deletions
@@ -26,6 +26,36 @@ picoclaw cron add --name "Daily summary" --message "Summarize today's logs" --cr
 picoclaw cron add --name "Ping" --message "heartbeat" --every 300 --deliver
 ```

+## Agent Tool Actions
+
+The agent-facing `cron` tool supports these actions:
+
+- `add`: create a new job.
+- `list`: show job names, ids, and schedules.
+- `get`: fetch one full persisted job by `job_id`, including its saved payload.
+- `update`: partially update one job by `job_id`; omitted fields are preserved.
+- `remove`, `enable`, `disable`: existing management actions.
+
+When rescheduling an existing task, use `list -> get -> update`. Do not use
+`remove -> add` just to change the schedule, because recreating a job can drop
+the original prompt, delivery target, or command payload.
+
+Example tool calls:
+
+```json
+{"action":"get","job_id":"79095b2f5685a0f2"}
+```
+
+```json
+{"action":"update","job_id":"79095b2f5685a0f2","cron_expr":"30 10 * * *"}
+```
+
+`update` accepts `name`, `message`, `command`, and exactly one schedule field
+(`at_seconds`, `every_seconds`, or `cron_expr`).
+Omit `command` to preserve it, set `command` to a non-empty string to replace
+it, or set `command` to `""` to clear it. Command updates require the same
+internal channel and confirmation gates as command creation.
+
 ## Execution Modes

 Jobs are stored with a message payload and can execute in three stable user-facing modes:
@@ -447,14 +447,37 @@ func (cs *CronService) AddJob(
 	return &job, nil
 }

+func (cs *CronService) GetJob(jobID string) (*CronJob, bool) {
+	cs.mu.RLock()
+	defer cs.mu.RUnlock()
+
+	for i := range cs.store.Jobs {
+		if cs.store.Jobs[i].ID == jobID {
+			jobCopy := cloneCronJob(cs.store.Jobs[i])
+			return &jobCopy, true
+		}
+	}
+	return nil, false
+}
+
 func (cs *CronService) UpdateJob(job *CronJob) error {
 	cs.mu.Lock()
 	defer cs.mu.Unlock()

 	for i := range cs.store.Jobs {
 		if cs.store.Jobs[i].ID == job.ID {
-			cs.store.Jobs[i] = *job
-			cs.store.Jobs[i].UpdatedAtMS = time.Now().UnixMilli()
+			previous := cs.store.Jobs[i]
+			updated := cloneCronJob(*job)
+			now := time.Now().UnixMilli()
+			updated.UpdatedAtMS = now
+			if updated.Enabled {
+				if previous.Enabled != updated.Enabled || !sameSchedule(previous.Schedule, updated.Schedule) {
+					updated.State.NextRunAtMS = cs.computeNextRun(&updated.Schedule, now)
+				}
+			} else {
+				updated.State.NextRunAtMS = nil
+			}
+			cs.store.Jobs[i] = updated

 			cs.notify()

@@ -464,6 +487,42 @@ func (cs *CronService) UpdateJob(job *CronJob) error {
 	return fmt.Errorf("job not found")
 }

+func cloneCronJob(job CronJob) CronJob {
+	clone := job
+	if job.Schedule.AtMS != nil {
+		atMS := *job.Schedule.AtMS
+		clone.Schedule.AtMS = &atMS
+	}
+	if job.Schedule.EveryMS != nil {
+		everyMS := *job.Schedule.EveryMS
+		clone.Schedule.EveryMS = &everyMS
+	}
+	if job.State.NextRunAtMS != nil {
+		nextRunAtMS := *job.State.NextRunAtMS
+		clone.State.NextRunAtMS = &nextRunAtMS
+	}
+	if job.State.LastRunAtMS != nil {
+		lastRunAtMS := *job.State.LastRunAtMS
+		clone.State.LastRunAtMS = &lastRunAtMS
+	}
+	return clone
+}
+
+func sameSchedule(a, b CronSchedule) bool {
+	return a.Kind == b.Kind &&
+		sameInt64(a.AtMS, b.AtMS) &&
+		sameInt64(a.EveryMS, b.EveryMS) &&
+		a.Expr == b.Expr &&
+		a.TZ == b.TZ
+}
+
+func sameInt64(a, b *int64) bool {
+	if a == nil || b == nil {
+		return a == b
+	}
+	return *a == *b
+}
+
 func (cs *CronService) RemoveJob(jobID string) bool {
 	cs.mu.Lock()
 	defer cs.mu.Unlock()
@@ -82,6 +82,136 @@ func TestCronService_CRUD(t *testing.T) {
 	}
 }

+func TestCronService_GetJobReturnsCopy(t *testing.T) {
+	cs, path := setupService(nil)
+	defer os.Remove(path)
+
+	everyMS := int64(60_000)
+	job, err := cs.AddJob("Task1", CronSchedule{Kind: "every", EveryMS: &everyMS}, "msg", "ch", "to")
+	if err != nil {
+		t.Fatalf("AddJob failed: %v", err)
+	}
+	if job.State.NextRunAtMS == nil {
+		t.Fatal("expected initial next run")
+	}
+	nextRun := *job.State.NextRunAtMS
+
+	got, ok := cs.GetJob(job.ID)
+	if !ok {
+		t.Fatal("GetJob should find job")
+	}
+	got.Name = "mutated"
+	got.Payload.Message = "changed"
+	if got.Schedule.EveryMS != nil {
+		*got.Schedule.EveryMS = 120_000
+	}
+	if got.State.NextRunAtMS != nil {
+		*got.State.NextRunAtMS = time.Now().Add(3 * time.Hour).UnixMilli()
+	}
+
+	again, ok := cs.GetJob(job.ID)
+	if !ok {
+		t.Fatal("GetJob should still find job")
+	}
+	if again.Name != "Task1" || again.Payload.Message != "msg" {
+		t.Fatalf("GetJob should return a copy, got %+v", again)
+	}
+	if again.Schedule.EveryMS == nil || *again.Schedule.EveryMS != everyMS {
+		t.Fatalf("GetJob should not alias schedule pointers, got %+v", again.Schedule)
+	}
+	if again.State.NextRunAtMS == nil || *again.State.NextRunAtMS != nextRun {
+		t.Fatalf("GetJob should not alias state pointers, got %+v", again.State)
+	}
+}
+
+func TestCronService_UpdateJobRecomputesNextRunOnScheduleOrEnabledChange(t *testing.T) {
+	cs, path := setupService(nil)
+	defer os.Remove(path)
+
+	at := time.Now().Add(time.Hour).UnixMilli()
+	job, err := cs.AddJob("Task1", CronSchedule{Kind: "at", AtMS: &at}, "msg", "ch", "to")
+	if err != nil {
+		t.Fatalf("AddJob failed: %v", err)
+	}
+	if job.State.NextRunAtMS == nil {
+		t.Fatal("expected initial next run")
+	}
+	initialNextRun := *job.State.NextRunAtMS
+
+	everyMS := int64(30_000)
+	job.Schedule = CronSchedule{Kind: "every", EveryMS: &everyMS}
+	if err := cs.UpdateJob(job); err != nil {
+		t.Fatalf("UpdateJob schedule failed: %v", err)
+	}
+	updated, ok := cs.GetJob(job.ID)
+	if !ok {
+		t.Fatal("updated job not found")
+	}
+	if updated.State.NextRunAtMS == nil {
+		t.Fatal("expected recomputed next run after schedule change")
+	}
+	if *updated.State.NextRunAtMS == initialNextRun {
+		t.Fatalf("next run should be recomputed, still %d", initialNextRun)
+	}
+
+	if disabled := cs.EnableJob(job.ID, false); disabled == nil {
+		t.Fatal("EnableJob(false) returned nil")
+	}
+	disabled, ok := cs.GetJob(job.ID)
+	if !ok {
+		t.Fatal("disabled job not found")
+	}
+	disabled.Enabled = true
+	if err := cs.UpdateJob(disabled); err != nil {
+		t.Fatalf("UpdateJob enabled failed: %v", err)
+	}
+	reenabled, ok := cs.GetJob(job.ID)
+	if !ok {
+		t.Fatal("reenabled job not found")
+	}
+	if !reenabled.Enabled || reenabled.State.NextRunAtMS == nil {
+		t.Fatalf("expected enabled job with next run, got %+v", reenabled)
+	}
+}
+
+func TestCronService_UpdateJobPreservesRunStateOnPayloadOnlyChange(t *testing.T) {
+	cs, path := setupService(nil)
+	defer os.Remove(path)
+
+	everyMS := int64(60_000)
+	job, err := cs.AddJob("Task1", CronSchedule{Kind: "every", EveryMS: &everyMS}, "msg", "ch", "to")
+	if err != nil {
+		t.Fatalf("AddJob failed: %v", err)
+	}
+	lastRun := time.Now().Add(-time.Minute).UnixMilli()
+	job.State.LastRunAtMS = &lastRun
+	job.State.LastStatus = "ok"
+	job.State.LastError = "previous"
+	if job.State.NextRunAtMS == nil {
+		t.Fatal("expected next run before update")
+	}
+	nextRun := *job.State.NextRunAtMS
+
+	job.Payload.Message = "updated msg"
+	if err := cs.UpdateJob(job); err != nil {
+		t.Fatalf("UpdateJob failed: %v", err)
+	}
+
+	updated, ok := cs.GetJob(job.ID)
+	if !ok {
+		t.Fatal("updated job not found")
+	}
+	if updated.State.LastRunAtMS == nil || *updated.State.LastRunAtMS != lastRun {
+		t.Fatalf("last run changed: %+v", updated.State)
+	}
+	if updated.State.LastStatus != "ok" || updated.State.LastError != "previous" {
+		t.Fatalf("last status changed: %+v", updated.State)
+	}
+	if updated.State.NextRunAtMS == nil || *updated.State.NextRunAtMS != nextRun {
+		t.Fatalf("next run should be preserved: before=%d after=%+v", nextRun, updated.State.NextRunAtMS)
+	}
+}
+
 // 2. Test Cron Expression Calculation Logic
 func TestCronService_ComputeNextRun(t *testing.T) {
 	cs, path := setupService(nil)
@@ -2,6 +2,7 @@ package tools

 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strings"
 	"time"
@@ -75,7 +76,12 @@ func (t *CronTool) Name() string {

 // Description returns the tool description
 func (t *CronTool) Description() string {
-	return "Schedule reminders, tasks, or system commands. IMPORTANT: When user asks to be reminded or scheduled, you MUST call this tool. Use 'at_seconds' for one-time reminders (e.g., 'remind me in 10 minutes' → at_seconds=600). Use 'every_seconds' ONLY for recurring tasks (e.g., 'every 2 hours' → every_seconds=7200). Use 'cron_expr' for complex recurring schedules. Use 'command' to execute shell commands directly."
+	return `Schedule, inspect, and update reminders, tasks, or system commands. 
+IMPORTANT: When user asks to be reminded or scheduled, you MUST call this tool. 
+Use 'at_seconds' for one-time reminders (e.g., 'remind me in 10 minutes' → at_seconds=600). 
+Use 'every_seconds' ONLY for recurring tasks (e.g., 'every 2 hours' → every_seconds=7200). 
+Use 'cron_expr' for complex recurring schedules. 
+Use 'command' to execute shell commands directly.`
 }

 // Parameters returns the tool parameters schema
@@ -85,8 +91,12 @@ func (t *CronTool) Parameters() map[string]any {
 		"properties": map[string]any{
 			"action": map[string]any{
 				"type":        "string",
-				"enum":        []string{"add", "list", "remove", "enable", "disable"},
-				"description": "Action to perform. Use 'add' when user wants to schedule a reminder or task.",
+				"enum":        []string{"add", "list", "get", "update", "remove", "enable", "disable"},
+				"description": "Action to perform. Use 'get' before editing and 'update' to change existing jobs without losing their payload.",
+			},
+			"name": map[string]any{
+				"type":        "string",
+				"description": "Optional job display name for update or add.",
 			},
 			"message": map[string]any{
 				"type":        "string",
@@ -94,7 +104,7 @@ func (t *CronTool) Parameters() map[string]any {
 			},
 			"command": map[string]any{
 				"type":        "string",
-				"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message.",
+				"description": "Optional: Shell command to execute directly (e.g., 'df -h'). If set, the agent will run this command and report output instead of just showing the message. For update, omit to preserve the command or pass an empty string to clear it.",
 			},
 			"command_confirm": map[string]any{
 				"type":        "boolean",
@@ -114,7 +124,7 @@ func (t *CronTool) Parameters() map[string]any {
 			},
 			"job_id": map[string]any{
 				"type":        "string",
-				"description": "Job ID (for remove/enable/disable)",
+				"description": "Job ID (for get/update/remove/enable/disable)",
 			},
 		},
 		"required": []string{"action"},
@@ -133,6 +143,10 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *ToolResult
 		return t.addJob(ctx, args)
 	case "list":
 		return t.listJobs()
+	case "get":
+		return t.getJob(ctx, args)
+	case "update":
+		return t.updateJob(ctx, args)
 	case "remove":
 		return t.removeJob(args)
 	case "enable":
@@ -262,6 +276,81 @@ func (t *CronTool) listJobs() *ToolResult {
 	return SilentResult(result.String())
 }

+func (t *CronTool) getJob(ctx context.Context, args map[string]any) *ToolResult {
+	jobID, errResult := requiredCronJobID(args, "get")
+	if errResult != nil {
+		return errResult
+	}
+
+	job, ok := t.cronService.GetJob(jobID)
+	if !ok {
+		return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
+	}
+
+	return SilentResult(formatCronJobJSON(job))
+}
+
+func (t *CronTool) updateJob(ctx context.Context, args map[string]any) *ToolResult {
+	jobID, errResult := requiredCronJobID(args, "update")
+	if errResult != nil {
+		return errResult
+	}
+
+	job, ok := t.cronService.GetJob(jobID)
+	if !ok {
+		return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
+	}
+
+	patches := 0
+
+	if name, present, errResult := optionalNonEmptyString(args, "name"); errResult != nil {
+		return errResult
+	} else if present {
+		job.Name = name
+		patches++
+	}
+
+	if message, present, errResult := optionalNonEmptyString(args, "message"); errResult != nil {
+		return errResult
+	} else if present {
+		job.Payload.Message = message
+		patches++
+	}
+
+	schedule, hasSchedule, errResult := schedulePatch(args)
+	if errResult != nil {
+		return errResult
+	}
+	if hasSchedule {
+		job.Schedule = schedule
+		job.DeleteAfterRun = schedule.Kind == "at"
+		patches++
+	}
+
+	command, commandPresent, errResult := optionalString(args, "command")
+	if errResult != nil {
+		return errResult
+	}
+	if commandPresent {
+		if errResult := t.validateCommandMutation(ctx, args); errResult != nil {
+			return errResult
+		}
+		job.Payload.Command = command
+		patches++
+	}
+
+	if patches == 0 {
+		return ErrorResult("at least one update field is required")
+	}
+
+	if err := t.cronService.UpdateJob(job); err != nil {
+		return ErrorResult(fmt.Sprintf("Error updating job: %v", err))
+	}
+
+	updated, _ := t.cronService.GetJob(jobID)
+	return SilentResult(fmt.Sprintf("Cron job updated:\n%s", formatCronJobJSON(updated)))
+}
+
 func (t *CronTool) removeJob(args map[string]any) *ToolResult {
 	jobID, ok := args["job_id"].(string)
 	if !ok || jobID == "" {
@@ -274,6 +363,127 @@ func (t *CronTool) removeJob(args map[string]any) *ToolResult {
 	return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
 }

+func requiredCronJobID(args map[string]any, action string) (string, *ToolResult) {
+	jobID, ok := args["job_id"].(string)
+	if !ok || jobID == "" {
+		return "", ErrorResult(fmt.Sprintf("job_id is required for %s", action))
+	}
+	return jobID, nil
+}
+
+func optionalNonEmptyString(args map[string]any, key string) (string, bool, *ToolResult) {
+	_, present := args[key]
+	if !present {
+		return "", false, nil
+	}
+	text, _, errResult := optionalString(args, key)
+	if errResult != nil {
+		return "", false, errResult
+	}
+	if strings.TrimSpace(text) == "" {
+		return "", false, ErrorResult(fmt.Sprintf("%s cannot be empty", key))
+	}
+	return text, true, nil
+}
+
+func optionalString(args map[string]any, key string) (string, bool, *ToolResult) {
+	value, present := args[key]
+	if !present {
+		return "", false, nil
+	}
+	text, ok := value.(string)
+	if !ok {
+		return "", false, ErrorResult(fmt.Sprintf("%s must be a string", key))
+	}
+	return text, true, nil
+}
+
+func schedulePatch(args map[string]any) (cron.CronSchedule, bool, *ToolResult) {
+	var schedule cron.CronSchedule
+	patches := 0
+
+	if _, present := args["at_seconds"]; present {
+		seconds, errResult := positiveSeconds(args, "at_seconds")
+		if errResult != nil {
+			return cron.CronSchedule{}, false, errResult
+		}
+		atMS := time.Now().UnixMilli() + seconds*1000
+		schedule = cron.CronSchedule{Kind: "at", AtMS: &atMS}
+		patches++
+	}
+
+	if _, present := args["every_seconds"]; present {
+		seconds, errResult := positiveSeconds(args, "every_seconds")
+		if errResult != nil {
+			return cron.CronSchedule{}, false, errResult
+		}
+		everyMS := seconds * 1000
+		schedule = cron.CronSchedule{Kind: "every", EveryMS: &everyMS}
+		patches++
+	}
+
+	if _, present := args["cron_expr"]; present {
+		cronExpr, ok := args["cron_expr"].(string)
+		if !ok {
+			return cron.CronSchedule{}, false, ErrorResult("cron_expr must be a string")
+		}
+		if strings.TrimSpace(cronExpr) == "" {
+			return cron.CronSchedule{}, false, ErrorResult("cron_expr cannot be empty")
+		}
+		schedule = cron.CronSchedule{Kind: "cron", Expr: cronExpr}
+		patches++
+	}
+
+	if patches > 1 {
+		return cron.CronSchedule{}, false, ErrorResult("only one of at_seconds, every_seconds, or cron_expr can be set")
+	}
+	return schedule, patches == 1, nil
+}
+
+func positiveSeconds(args map[string]any, key string) (int64, *ToolResult) {
+	value := args[key]
+	var seconds int64
+	switch v := value.(type) {
+	case float64:
+		if v != float64(int64(v)) {
+			return 0, ErrorResult(fmt.Sprintf("%s must be a positive integer", key))
+		}
+		seconds = int64(v)
+	case int:
+		seconds = int64(v)
+	case int64:
+		seconds = v
+	default:
+		return 0, ErrorResult(fmt.Sprintf("%s must be a positive integer", key))
+	}
+	if seconds <= 0 {
+		return 0, ErrorResult(fmt.Sprintf("%s must be a positive integer", key))
+	}
+	return seconds, nil
+}
+
+func (t *CronTool) validateCommandMutation(ctx context.Context, args map[string]any) *ToolResult {
+	if !t.execEnabled {
+		return ErrorResult("command execution is disabled")
+	}
+	if !constants.IsInternalChannel(ToolChannel(ctx)) {
+		return ErrorResult("updating command execution is restricted to internal channels")
+	}
+	commandConfirm, _ := args["command_confirm"].(bool)
+	if !t.allowCommand && !commandConfirm {
+		return ErrorResult("command_confirm=true is required when allow_command is disabled")
+	}
+	return nil
+}
+
+func formatCronJobJSON(job *cron.CronJob) string {
+	data, err := json.Marshal(job)
+	if err != nil {
+		return fmt.Sprintf("%+v", *job)
+	}
+	return string(data)
+}
+
 func (t *CronTool) enableJob(args map[string]any, enable bool) *ToolResult {
 	jobID, ok := args["job_id"].(string)
 	if !ok || jobID == "" {
@@ -2,6 +2,7 @@ package tools

 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"path/filepath"
 	"strings"
@@ -73,6 +74,19 @@ func newTestCronTool(t *testing.T) *CronTool {
 	return newTestCronToolWithConfig(t, config.DefaultConfig())
 }

+func parseCronJobResult(t *testing.T, result *ToolResult) cron.CronJob {
+	t.Helper()
+	text := result.ForLLM
+	if idx := strings.Index(text, "{"); idx >= 0 {
+		text = text[idx:]
+	}
+	var job cron.CronJob
+	if err := json.Unmarshal([]byte(text), &job); err != nil {
+		t.Fatalf("failed to parse cron job JSON %q: %v", result.ForLLM, err)
+	}
+	return job
+}
+
 // TestCronTool_CommandBlockedFromRemoteChannel verifies command scheduling is restricted to internal channels
 func TestCronTool_CommandBlockedFromRemoteChannel(t *testing.T) {
 	tool := newTestCronTool(t)
@@ -231,6 +245,323 @@ func TestCronTool_NonCommandJobAllowedFromRemoteChannel(t *testing.T) {
 	}
 }

+func TestCronTool_GetReturnsFullJobPayload(t *testing.T) {
+	tool := newTestCronTool(t)
+	ctx := WithToolContext(context.Background(), "telegram", "chat-1")
+	everyMS := int64(60_000)
+	message := strings.Repeat("daily briefing details ", 8)
+	job, err := tool.cronService.AddJob(
+		"daily",
+		cron.CronSchedule{Kind: "every", EveryMS: &everyMS},
+		message,
+		"telegram",
+		"chat-1",
+	)
+	if err != nil {
+		t.Fatalf("AddJob() error: %v", err)
+	}
+
+	result := tool.Execute(ctx, map[string]any{
+		"action": "get",
+		"job_id": job.ID,
+	})
+
+	if result.IsError {
+		t.Fatalf("get failed: %s", result.ForLLM)
+	}
+	got := parseCronJobResult(t, result)
+	if got.ID != job.ID || got.Payload.Message != message || got.Payload.Channel != "telegram" ||
+		got.Payload.To != "chat-1" {
+		t.Fatalf("get returned wrong payload: %+v", got)
+	}
+	if got.Schedule.Kind != "every" || got.Schedule.EveryMS == nil || *got.Schedule.EveryMS != everyMS {
+		t.Fatalf("get returned wrong schedule: %+v", got.Schedule)
+	}
+	if got.State.NextRunAtMS == nil {
+		t.Fatal("get should include next run state")
+	}
+}
+
+func TestCronTool_UpdateSchedulePreservesPayload(t *testing.T) {
+	tool := newTestCronTool(t)
+	ctx := WithToolContext(context.Background(), "cli", "direct")
+	original, err := tool.cronService.AddJob(
+		"AI daily",
+		cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
+		"fetch RSS, include source links",
+		"weixin",
+		"chat-1",
+	)
+	if err != nil {
+		t.Fatalf("AddJob() error: %v", err)
+	}
+
+	result := tool.Execute(ctx, map[string]any{
+		"action":    "update",
+		"job_id":    original.ID,
+		"cron_expr": "30 10 * * *",
+	})
+
+	if result.IsError {
+		t.Fatalf("update failed: %s", result.ForLLM)
+	}
+	updated, ok := tool.cronService.GetJob(original.ID)
+	if !ok {
+		t.Fatal("updated job not found")
+	}
+	if updated.ID != original.ID || updated.CreatedAtMS != original.CreatedAtMS {
+		t.Fatalf("identity changed after update: before=%+v after=%+v", original, updated)
+	}
+	if updated.Payload.Message != original.Payload.Message || updated.Payload.Channel != original.Payload.Channel ||
+		updated.Payload.To != original.Payload.To {
+		t.Fatalf("payload was not preserved: %+v", updated.Payload)
+	}
+	if updated.Schedule.Kind != "cron" || updated.Schedule.Expr != "30 10 * * *" {
+		t.Fatalf("schedule not updated: %+v", updated.Schedule)
+	}
+	if updated.DeleteAfterRun {
+		t.Fatal("cron schedule should not delete after run")
+	}
+}
+
+func TestCronTool_UpdateMessagePreservesScheduleAndNextRun(t *testing.T) {
+	tool := newTestCronTool(t)
+	ctx := WithToolContext(context.Background(), "telegram", "chat-1")
+	everyMS := int64(120_000)
+	original, err := tool.cronService.AddJob(
+		"reminder",
+		cron.CronSchedule{Kind: "every", EveryMS: &everyMS},
+		"old message",
+		"telegram",
+		"chat-1",
+	)
+	if err != nil {
+		t.Fatalf("AddJob() error: %v", err)
+	}
+	if original.State.NextRunAtMS == nil {
+		t.Fatal("expected original next run")
+	}
+	nextRunBefore := *original.State.NextRunAtMS
+
+	result := tool.Execute(ctx, map[string]any{
+		"action":  "update",
+		"job_id":  original.ID,
+		"message": "new message",
+	})
+
+	if result.IsError {
+		t.Fatalf("update failed: %s", result.ForLLM)
+	}
+	updated, _ := tool.cronService.GetJob(original.ID)
+	if updated.Payload.Message != "new message" {
+		t.Fatalf("message not updated: %+v", updated.Payload)
+	}
+	if updated.Name != "reminder" {
+		t.Fatalf("name should be preserved, got %q", updated.Name)
+	}
+	if updated.Schedule.Kind != "every" || updated.Schedule.EveryMS == nil || *updated.Schedule.EveryMS != everyMS {
+		t.Fatalf("schedule should be preserved: %+v", updated.Schedule)
+	}
+	if updated.State.NextRunAtMS == nil || *updated.State.NextRunAtMS != nextRunBefore {
+		t.Fatalf("next run should be preserved: before=%d after=%v", nextRunBefore, updated.State.NextRunAtMS)
+	}
+}
+
+func TestCronTool_UpdateValidationErrors(t *testing.T) {
+	tool := newTestCronTool(t)
+	ctx := WithToolContext(context.Background(), "cli", "direct")
+	job, err := tool.cronService.AddJob(
+		"job",
+		cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
+		"message",
+		"cli",
+		"direct",
+	)
+	if err != nil {
+		t.Fatalf("AddJob() error: %v", err)
+	}
+
+	tests := []struct {
+		name string
+		args map[string]any
+		want string
+	}{
+		{
+			name: "invalid job id",
+			args: map[string]any{"action": "update", "job_id": "missing", "message": "new"},
+			want: "not found",
+		},
+		{
+			name: "missing patch",
+			args: map[string]any{"action": "update", "job_id": job.ID},
+			want: "at least one update field",
+		},
+		{
+			name: "multiple schedule fields",
+			args: map[string]any{
+				"action":        "update",
+				"job_id":        job.ID,
+				"every_seconds": float64(60),
+				"cron_expr":     "0 9 * * *",
+			},
+			want: "only one of",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tool.Execute(ctx, tt.args)
+			if !result.IsError {
+				t.Fatalf("expected error, got: %s", result.ForLLM)
+			}
+			if !strings.Contains(result.ForLLM, tt.want) {
+				t.Fatalf("error = %q, want substring %q", result.ForLLM, tt.want)
+			}
+		})
+	}
+}
+
+func TestCronTool_RemoteCannotAccessOtherChatJob(t *testing.T) {
+	tool := newTestCronTool(t)
+	job, err := tool.cronService.AddJob(
+		"private",
+		cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
+		"secret",
+		"telegram",
+		"chat-1",
+	)
+	if err != nil {
+		t.Fatalf("AddJob() error: %v", err)
+	}
+	ctx := WithToolContext(context.Background(), "telegram", "chat-2")
+
+	getResult := tool.Execute(ctx, map[string]any{"action": "get", "job_id": job.ID})
+	if !getResult.IsError || !strings.Contains(getResult.ForLLM, "not accessible") {
+		t.Fatalf("expected inaccessible get, got: %+v", getResult)
+	}
+
+	updateResult := tool.Execute(ctx, map[string]any{"action": "update", "job_id": job.ID, "message": "changed"})
+	if !updateResult.IsError || !strings.Contains(updateResult.ForLLM, "not accessible") {
+		t.Fatalf("expected inaccessible update, got: %+v", updateResult)
+	}
+}
+
+func TestCronTool_RemoteCannotAccessCommandJob(t *testing.T) {
+	tool := newTestCronTool(t)
+	job, err := tool.cronService.AddJob(
+		"command",
+		cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
+		"run command",
+		"telegram",
+		"chat-1",
+	)
+	if err != nil {
+		t.Fatalf("AddJob() error: %v", err)
+	}
+	job.Payload.Command = "df -h"
+	if err := tool.cronService.UpdateJob(job); err != nil {
+		t.Fatalf("UpdateJob() error: %v", err)
+	}
+	ctx := WithToolContext(context.Background(), "telegram", "chat-1")
+
+	getResult := tool.Execute(ctx, map[string]any{"action": "get", "job_id": job.ID})
+	if !getResult.IsError || !strings.Contains(getResult.ForLLM, "not accessible") {
+		t.Fatalf("expected inaccessible get, got: %+v", getResult)
+	}
+
+	updateResult := tool.Execute(ctx, map[string]any{"action": "update", "job_id": job.ID, "message": "changed"})
+	if !updateResult.IsError || !strings.Contains(updateResult.ForLLM, "not accessible") {
+		t.Fatalf("expected inaccessible update, got: %+v", updateResult)
+	}
+}
+
+func TestCronTool_CommandUpdateSafetyGates(t *testing.T) {
+	t.Run("exec disabled", func(t *testing.T) {
+		cfg := config.DefaultConfig()
+		cfg.Tools.Exec.Enabled = false
+		tool := newTestCronToolWithConfig(t, cfg)
+		ctx := WithToolContext(context.Background(), "cli", "direct")
+		job, err := tool.cronService.AddJob(
+			"job",
+			cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
+			"message",
+			"cli",
+			"direct",
+		)
+		if err != nil {
+			t.Fatalf("AddJob() error: %v", err)
+		}
+
+		result := tool.Execute(ctx, map[string]any{
+			"action":          "update",
+			"job_id":          job.ID,
+			"command":         "df -h",
+			"command_confirm": true,
+		})
+
+		if !result.IsError || !strings.Contains(result.ForLLM, "command execution is disabled") {
+			t.Fatalf("expected exec disabled error, got: %+v", result)
+		}
+	})
+
+	t.Run("confirm required", func(t *testing.T) {
+		cfg := config.DefaultConfig()
+		cfg.Tools.Cron.AllowCommand = false
+		tool := newTestCronToolWithConfig(t, cfg)
+		ctx := WithToolContext(context.Background(), "cli", "direct")
+		job, err := tool.cronService.AddJob(
+			"job",
+			cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
+			"message",
+			"cli",
+			"direct",
+		)
+		if err != nil {
+			t.Fatalf("AddJob() error: %v", err)
+		}
+
+		result := tool.Execute(ctx, map[string]any{
+			"action":  "update",
+			"job_id":  job.ID,
+			"command": "df -h",
+		})
+
+		if !result.IsError || !strings.Contains(result.ForLLM, "command_confirm=true") {
+			t.Fatalf("expected confirm error, got: %+v", result)
+		}
+
+		result = tool.Execute(ctx, map[string]any{
+			"action":          "update",
+			"job_id":          job.ID,
+			"command":         "df -h",
+			"command_confirm": true,
+		})
+
+		if result.IsError {
+			t.Fatalf("expected confirmed command update to succeed, got: %s", result.ForLLM)
+		}
+		updated, _ := tool.cronService.GetJob(job.ID)
+		if updated.Payload.Command != "df -h" {
+			t.Fatalf("command not updated: %+v", updated.Payload)
+		}
+
+		result = tool.Execute(ctx, map[string]any{
+			"action":          "update",
+			"job_id":          job.ID,
+			"command":         "",
+			"command_confirm": true,
+		})
+
+		if result.IsError {
+			t.Fatalf("expected empty command update to clear command, got: %s", result.ForLLM)
+		}
+		updated, _ = tool.cronService.GetJob(job.ID)
+		if updated.Payload.Command != "" {
+			t.Fatalf("command not cleared: %+v", updated.Payload)
+		}
+	})
+}
+
 func TestCronTool_ExecuteJobPublishesErrorWhenExecDisabled(t *testing.T) {
 	cfg := config.DefaultConfig()
 	cfg.Tools.Exec.Enabled = false