diff --git a/docs/reference/cron.md b/docs/reference/cron.md index 1d96fbad1..6808f5770 100644 --- a/docs/reference/cron.md +++ b/docs/reference/cron.md @@ -31,15 +31,20 @@ picoclaw cron add --name "Ping" --message "heartbeat" --every 300 --deliver 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. +- `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 diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 989749e9a..4e32797a6 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -92,7 +92,7 @@ func (t *CronTool) Parameters() map[string]any { "action": map[string]any{ "type": "string", "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.", + "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", @@ -142,7 +142,7 @@ 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": @@ -250,9 +250,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") } @@ -286,6 +294,9 @@ func (t *CronTool) getJob(ctx context.Context, args map[string]any) *ToolResult 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)) } @@ -300,6 +311,9 @@ func (t *CronTool) updateJob(ctx context.Context, args map[string]any) *ToolResu 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 @@ -476,6 +490,21 @@ func (t *CronTool) validateCommandMutation(ctx context.Context, args map[string] 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 { diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go index 63a34590f..41048c7f7 100644 --- a/pkg/tools/cron_test.go +++ b/pkg/tools/cron_test.go @@ -421,6 +421,71 @@ func TestCronTool_UpdateValidationErrors(t *testing.T) { } } +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( @@ -444,6 +509,13 @@ func TestCronTool_RemoteCannotAccessOtherChatJob(t *testing.T) { 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) { @@ -473,6 +545,13 @@ func TestCronTool_RemoteCannotAccessCommandJob(t *testing.T) { 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) { @@ -562,6 +641,50 @@ func TestCronTool_CommandUpdateSafetyGates(t *testing.T) { }) } +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