diff --git a/docs/reference/cron.md b/docs/reference/cron.md index 6483fa1370..6808f57708 100644 --- a/docs/reference/cron.md +++ b/docs/reference/cron.md @@ -26,6 +26,41 @@ 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 accessible job names, ids, and schedules. +- `get`: fetch one accessible persisted job by `job_id`, including its saved payload. +- `update`: partially update one accessible 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. + +Remote channel access is scoped to the current `channel/chat_id`: remote callers +can only list, get, or update jobs whose saved `payload.channel` and `payload.to` +match the current conversation. Command jobs include a shell command payload, so +they can only be listed, inspected, or updated from internal channels. + +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: diff --git a/pkg/cron/service.go b/pkg/cron/service.go index 6a8728943c..1f22f70875 100644 --- a/pkg/cron/service.go +++ b/pkg/cron/service.go @@ -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() diff --git a/pkg/cron/service_test.go b/pkg/cron/service_test.go index 6dff3b3873..3d003440bf 100644 --- a/pkg/cron/service_test.go +++ b/pkg/cron/service_test.go @@ -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) diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 78e581e778..b3b14dc480 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -2,6 +2,7 @@ package tools import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -75,18 +76,29 @@ 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 +// +//nolint:dupl // Tool parameter schemas intentionally use similar JSON-schema map literals. func (t *CronTool) Parameters() map[string]any { return map[string]any{ "type": "object", "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. Remote channels can only list/get/update jobs for the current channel/chat_id.", + }, + "name": map[string]any{ + "type": "string", + "description": "Optional job display name for update or add.", }, "message": map[string]any{ "type": "string", @@ -94,7 +106,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 +126,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"}, @@ -132,7 +144,11 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *ToolResult case "add": return t.addJob(ctx, args) case "list": - return t.listJobs() + return t.listJobs(ctx) + case "get": + return t.getJob(ctx, args) + case "update": + return t.updateJob(ctx, args) case "remove": return t.removeJob(args) case "enable": @@ -236,9 +252,17 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult return SilentResult(fmt.Sprintf("Cron job added: %s (id: %s)", job.Name, job.ID)) } -func (t *CronTool) listJobs() *ToolResult { +func (t *CronTool) listJobs(ctx context.Context) *ToolResult { jobs := t.cronService.ListJobs(false) + var accessibleJobs []cron.CronJob + for _, job := range jobs { + if t.canAccessJob(ctx, &job) { + accessibleJobs = append(accessibleJobs, job) + } + } + jobs = accessibleJobs + if len(jobs) == 0 { return SilentResult("No scheduled jobs") } @@ -262,6 +286,91 @@ 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)) + } + if !t.canAccessJob(ctx, job) { + return ErrorResult(fmt.Sprintf("Job %s is not accessible from this channel", 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)) + } + if !t.canAccessJob(ctx, job) { + return ErrorResult(fmt.Sprintf("Job %s is not accessible from this channel", jobID)) + } + + patches := 0 + + name, namePresent, nameErr := optionalNonEmptyString(args, "name") + if nameErr != nil { + return nameErr + } + if namePresent { + job.Name = name + patches++ + } + + message, messagePresent, messageErr := optionalNonEmptyString(args, "message") + if messageErr != nil { + return messageErr + } + if messagePresent { + 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 +383,142 @@ 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 (t *CronTool) canAccessJob(ctx context.Context, job *cron.CronJob) bool { + channel := ToolChannel(ctx) + if constants.IsInternalChannel(channel) { + return true + } + chatID := ToolChatID(ctx) + if channel == "" || chatID == "" { + return false + } + if job.Payload.Command != "" { + return false + } + return job.Payload.Channel == channel && job.Payload.To == chatID +} + +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 == "" { diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go index 9d9d17a001..41048c7f78 100644 --- a/pkg/tools/cron_test.go +++ b/pkg/tools/cron_test.go @@ -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,446 @@ 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_ListFiltersJobsForRemoteChannel(t *testing.T) { + tool := newTestCronTool(t) + ctx := WithToolContext(context.Background(), "telegram", "chat-1") + everyMS := int64(60_000) + + ownJob, err := tool.cronService.AddJob( + "own", + cron.CronSchedule{Kind: "every", EveryMS: &everyMS}, + "visible", + "telegram", + "chat-1", + ) + if err != nil { + t.Fatalf("AddJob() error: %v", err) + } + otherChatJob, err := tool.cronService.AddJob( + "other-chat", + cron.CronSchedule{Kind: "every", EveryMS: &everyMS}, + "hidden", + "telegram", + "chat-2", + ) + if err != nil { + t.Fatalf("AddJob() error: %v", err) + } + otherChannelJob, err := tool.cronService.AddJob( + "other-channel", + cron.CronSchedule{Kind: "every", EveryMS: &everyMS}, + "hidden", + "feishu", + "chat-1", + ) + if err != nil { + t.Fatalf("AddJob() error: %v", err) + } + commandJob, err := tool.cronService.AddJob( + "command", + cron.CronSchedule{Kind: "every", EveryMS: &everyMS}, + "hidden command", + "telegram", + "chat-1", + ) + if err != nil { + t.Fatalf("AddJob() error: %v", err) + } + commandJob.Payload.Command = "df -h" + if err := tool.cronService.UpdateJob(commandJob); err != nil { + t.Fatalf("UpdateJob() error: %v", err) + } + + result := tool.Execute(ctx, map[string]any{"action": "list"}) + + if result.IsError { + t.Fatalf("list failed: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, ownJob.ID) { + t.Fatalf("list should include own job %s, got: %s", ownJob.ID, result.ForLLM) + } + for _, hiddenID := range []string{otherChatJob.ID, otherChannelJob.ID, commandJob.ID} { + if strings.Contains(result.ForLLM, hiddenID) { + t.Fatalf("list should not include hidden job %s, got: %s", hiddenID, result.ForLLM) + } + } +} + +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) + } + unchanged, ok := tool.cronService.GetJob(job.ID) + if !ok { + t.Fatal("job should still exist") + } + if unchanged.Payload.Message != "secret" { + t.Fatalf("unauthorized update mutated job: %+v", unchanged.Payload) + } +} + +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) + } + unchanged, ok := tool.cronService.GetJob(job.ID) + if !ok { + t.Fatal("job should still exist") + } + if unchanged.Payload.Message != "run command" || unchanged.Payload.Command != "df -h" { + t.Fatalf("unauthorized update mutated command job: %+v", unchanged.Payload) + } +} + +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_InternalCanAccessCommandJobFromAnyChannel(t *testing.T) { + tool := newTestCronTool(t) + ctx := WithToolContext(context.Background(), "cli", "direct") + 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) + } + + getResult := tool.Execute(ctx, map[string]any{"action": "get", "job_id": job.ID}) + if getResult.IsError { + t.Fatalf("get failed: %s", getResult.ForLLM) + } + got := parseCronJobResult(t, getResult) + if got.Payload.Command != "df -h" || got.Payload.Channel != "telegram" || got.Payload.To != "chat-1" { + t.Fatalf("get returned wrong command job: %+v", got.Payload) + } + + updateResult := tool.Execute(ctx, map[string]any{ + "action": "update", + "job_id": job.ID, + "cron_expr": "30 10 * * *", + }) + if updateResult.IsError { + t.Fatalf("update failed: %s", updateResult.ForLLM) + } + updated, _ := tool.cronService.GetJob(job.ID) + if updated.Payload.Command != "df -h" { + t.Fatalf("command should be preserved: %+v", updated.Payload) + } + if updated.Schedule.Kind != "cron" || updated.Schedule.Expr != "30 10 * * *" { + t.Fatalf("schedule not updated: %+v", updated.Schedule) + } +} + func TestCronTool_ExecuteJobPublishesErrorWhenExecDisabled(t *testing.T) { cfg := config.DefaultConfig() cfg.Tools.Exec.Enabled = false diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 07626a638f..5c2b03f4f5 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -215,6 +215,7 @@ func (t *ExecTool) Description() string { return `Execute shell commands. Use background=true for long-running commands (returns sessionId). Use pty=true for interactive commands (can combine with background=true). Use poll/read/write/send-keys/kill with sessionId to manage background sessions. Sessions auto-cleanup 30 minutes after process exits; use kill to terminate early. Output buffer limit: 1MB.` } +//nolint:dupl // Tool parameter schemas intentionally use similar JSON-schema map literals. func (t *ExecTool) Parameters() map[string]any { return map[string]any{ "type": "object",