feat: add schedule tool for creating and managing cron jobs#523
feat: add schedule tool for creating and managing cron jobs#523envestcc wants to merge 2 commits intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new agent-facing schedule tool that wraps the existing cron service so the agent can create, list, and cancel scheduled jobs (one-time, interval, or cron-expression based), and wires the tool into the Picoclaw startup.
Changes:
- Added
pkg/tools/schedule.goimplementing thescheduletool (create,list,cancel) on top ofCronService. - Registered the new tool in
cmd/picoclaw/main.goalongside the existingcrontool.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/tools/schedule.go | Introduces the ScheduleTool implementation including argument schema, schedule parsing, and cron service calls. |
| cmd/picoclaw/main.go | Registers the ScheduleTool in setupCronTool() so it becomes available to the agent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Parse ISO datetime | ||
| atTime, err := time.Parse(time.RFC3339, atStr) | ||
| if err != nil { | ||
| // Try other common formats | ||
| atTime, err = time.Parse("2006-01-02T15:04:05", atStr) | ||
| if err != nil { | ||
| return cron.CronSchedule{}, fmt.Errorf("invalid at datetime format: use ISO format like '2024-01-01T12:00:00'") | ||
| } | ||
| } | ||
|
|
||
| // Apply timezone if specified | ||
| if tz != "" { | ||
| loc, err := time.LoadLocation(tz) | ||
| if err != nil { | ||
| return cron.CronSchedule{}, fmt.Errorf("invalid timezone: %v", err) | ||
| } |
There was a problem hiding this comment.
Timezone handling for kind: "at" is incorrect for datetimes without an explicit offset. time.Parse("2006-01-02T15:04:05", atStr) produces a UTC time, and then atTime.In(loc) converts the instant (shifting the wall clock time) instead of interpreting the input as local time in timezone. Use time.ParseInLocation (with loc when provided, otherwise time.Local) for layouts that don’t include a zone, and avoid converting the parsed instant after the fact.
| // Parse ISO datetime | |
| atTime, err := time.Parse(time.RFC3339, atStr) | |
| if err != nil { | |
| // Try other common formats | |
| atTime, err = time.Parse("2006-01-02T15:04:05", atStr) | |
| if err != nil { | |
| return cron.CronSchedule{}, fmt.Errorf("invalid at datetime format: use ISO format like '2024-01-01T12:00:00'") | |
| } | |
| } | |
| // Apply timezone if specified | |
| if tz != "" { | |
| loc, err := time.LoadLocation(tz) | |
| if err != nil { | |
| return cron.CronSchedule{}, fmt.Errorf("invalid timezone: %v", err) | |
| } | |
| // Determine location to use for parsing times without explicit offset | |
| var loc *time.Location | |
| if tz != "" { | |
| var err error | |
| loc, err = time.LoadLocation(tz) | |
| if err != nil { | |
| return cron.CronSchedule{}, fmt.Errorf("invalid timezone: %v", err) | |
| } | |
| } else { | |
| loc = time.Local | |
| } | |
| // Parse ISO datetime | |
| atTime, err := time.Parse(time.RFC3339, atStr) | |
| usedNaiveLayout := false | |
| if err != nil { | |
| // Try other common formats (without timezone, interpret in loc) | |
| atTime, err = time.ParseInLocation("2006-01-02T15:04:05", atStr, loc) | |
| if err != nil { | |
| return cron.CronSchedule{}, fmt.Errorf("invalid at datetime format: use ISO format like '2024-01-01T12:00:00'") | |
| } | |
| usedNaiveLayout = true | |
| } | |
| // Apply timezone if specified for offset-aware inputs | |
| if tz != "" && !usedNaiveLayout { |
| // Get timezone if specified | ||
| tz, _ := scheduleMap["timezone"].(string) | ||
| schedule.TZ = tz | ||
|
|
There was a problem hiding this comment.
The tool accepts a schedule.timezone value and stores it in schedule.TZ, but CronService.computeNextRun currently ignores CronSchedule.TZ entirely. As a result, cron schedules will not actually run in the requested timezone (and every doesn’t need a timezone). Either implement TZ support in the cron service’s next-run computation or remove/clarify the timezone parameter so the tool doesn’t advertise behavior it can’t deliver.
| everySeconds, ok := scheduleMap["every_seconds"].(float64) | ||
| if !ok { | ||
| return cron.CronSchedule{}, fmt.Errorf("every_seconds field is required for 'every' kind") | ||
| } | ||
| if everySeconds <= 0 { | ||
| return cron.CronSchedule{}, fmt.Errorf("every_seconds must be positive") | ||
| } | ||
| everyMS := int64(everySeconds) * 1000 | ||
| schedule.EveryMS = &everyMS |
There was a problem hiding this comment.
every_seconds parsing is too strict and can silently truncate. The schema declares it as an integer, but the implementation only accepts float64 and then casts to int64 (dropping any fractional part). Consider accepting common numeric types (e.g., int, int64, float64) and explicitly rejecting non-integer values to avoid surprising schedules.
| // parseSchedule parses the schedule configuration into a CronSchedule | ||
| func (t *ScheduleTool) parseSchedule(scheduleMap map[string]interface{}) (cron.CronSchedule, error) { | ||
| kind, ok := scheduleMap["kind"].(string) | ||
| if !ok { | ||
| return cron.CronSchedule{}, fmt.Errorf("kind is required in schedule") | ||
| } | ||
|
|
There was a problem hiding this comment.
No unit tests are added for the new ScheduleTool. Since it includes non-trivial argument parsing (schedule kinds, datetime parsing, timezone validation) and user-facing formatting, consider adding tests for create/list/cancel and schedule parsing edge cases (invalid kind/expr, negative intervals, timezone errors, naive vs RFC3339 timestamps).
envestcc
force-pushed
the
feat/schedule-tool
branch
from
5ea0883 to
6838efb
Compare
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Good feature addition with thorough test coverage (585 lines of tests for 322 lines of code). A few observations:
-
Cron expression validation — The
cronkind accepts any string inexprwithout validating it's a legal cron expression. If the underlyingCronService.AddJobdoesn't validate either, a malformed expression like* * * *(4 fields) would be silently accepted and then fail at runtime. Consider adding a parse check here or documenting that validation happens downstream. -
No minimum interval guard for
every—every_seconds: 1is accepted. A user (or an LLM) could create a job that fires every second, which could be abused for DoS-like behavior. Consider a minimum threshold (e.g., 30s or 60s). -
No "at" past-time check — Scheduling a one-time job for a time in the past will silently create a job that presumably fires immediately. Not necessarily a bug, but it might be worth either rejecting past times or documenting the behavior.
-
Context race condition —
SetContextwriteschannel/chatIDwith a write lock, andcreateJobreads with a read lock. This is correct for mutex safety, but semantically there's a TOCTOU issue: if two concurrent tool invocations from different chats callSetContextand thencreateJob, the secondSetContextcould overwrite the first before the firstcreateJobreads. Consider passing channel/chatID via the args instead of mutable state, or accepting this as a known limitation for single-user deployments. -
listJobsstring concatenation — Minor: this function still uses+=string concatenation in a loop. Given PR #524 is converting these, you might want to usestrings.Builderhere for consistency. -
setupCronToolin main.go — Missing blank line beforefunc loadConfig(). Cosmetic.
Overall well-structured. Tests cover all error paths and happy paths. LGTM with the above as optional improvements.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func setupCronTool(agentLoop *agent.AgentLoop, msgBus *bus.MessageBus, workspace string, restrict bool) *cron.CronService { | ||
| cronStorePath := filepath.Join(workspace, "cron", "jobs.json") | ||
|
|
||
| // Create cron service | ||
| cronService := cron.NewCronService(cronStorePath, nil) | ||
|
|
||
| // Create and register CronTool | ||
| cronTool := tools.NewCronTool(cronService, agentLoop, msgBus, workspace, restrict) | ||
| agentLoop.RegisterTool(cronTool) | ||
|
|
||
| // Create and register ScheduleTool for agent use | ||
| scheduleTool := tools.NewScheduleTool(cronService) | ||
| agentLoop.RegisterTool(scheduleTool) | ||
|
|
||
| // Set the onJob handler | ||
| cronService.SetOnJob(func(job *cron.CronJob) (string, error) { | ||
| result := cronTool.ExecuteJob(context.Background(), job) | ||
| return result, nil | ||
| }) | ||
|
|
||
| return cronService | ||
| } |
There was a problem hiding this comment.
This new setupCronTool() in main.go will not compile: (1) the package already defines setupCronTool in cmd_gateway.go (duplicate symbol in the same package), and (2) tools.NewCronTool is called with the wrong number of args (it currently requires execTimeout and *config.Config). Also, this file is missing the required imports for agent/bus/cron/tools/context. Consider deleting this function and instead updating/reusing the existing setupCronTool in cmd_gateway.go to register ScheduleTool.
| func setupCronTool(agentLoop *agent.AgentLoop, msgBus *bus.MessageBus, workspace string, restrict bool) *cron.CronService { | |
| cronStorePath := filepath.Join(workspace, "cron", "jobs.json") | |
| // Create cron service | |
| cronService := cron.NewCronService(cronStorePath, nil) | |
| // Create and register CronTool | |
| cronTool := tools.NewCronTool(cronService, agentLoop, msgBus, workspace, restrict) | |
| agentLoop.RegisterTool(cronTool) | |
| // Create and register ScheduleTool for agent use | |
| scheduleTool := tools.NewScheduleTool(cronService) | |
| agentLoop.RegisterTool(scheduleTool) | |
| // Set the onJob handler | |
| cronService.SetOnJob(func(job *cron.CronJob) (string, error) { | |
| result := cronTool.ExecuteJob(context.Background(), job) | |
| return result, nil | |
| }) | |
| return cronService | |
| } |
| "at": map[string]interface{}{ | ||
| "type": "string", | ||
| "description": "ISO datetime for 'at' kind (e.g., '2024-01-01T12:00:00'). Use format: YYYY-MM-DDTHH:MM:SS.", | ||
| }, | ||
| "every_seconds": map[string]interface{}{ | ||
| "type": "integer", | ||
| "description": "Interval in seconds for 'every' kind. Example: 3600 for every hour. Must be a positive integer.", | ||
| }, | ||
| "expr": map[string]interface{}{ | ||
| "type": "string", | ||
| "description": "Cron expression for 'cron' kind (e.g., '0 9 * * *' for daily at 9am). Format: min hour day month dow.", | ||
| }, | ||
| "timezone": map[string]interface{}{ | ||
| "type": "string", | ||
| "description": "Timezone for interpreting 'at' times without explicit offset (e.g., 'Asia/Shanghai', 'UTC'). Only applies to 'at' kind. If not specified, uses system timezone.", | ||
| }, |
There was a problem hiding this comment.
The tool schema uses schedule fields that don’t match the CronSchedule JSON tags / issue #351 examples (e.g., cron.CronSchedule expects atMs/everyMs/tz, while the tool exposes at/every_seconds/timezone). To reduce model confusion and better match the existing cron API, consider supporting aliases like schedule.atMs (int64 ms), schedule.everyMs, and schedule.tz in addition to (or instead of) the current fields.
| if tz != "" && !usedNaiveLayout { | ||
| atTime = atTime.In(loc) | ||
| } | ||
|
|
There was a problem hiding this comment.
For kind='at', past timestamps currently parse successfully and will create an enabled job with NextRunAtMS=nil (cron service won’t run it, and one-shot jobs won’t auto-delete). Consider validating that the parsed time is in the future (e.g., > time.Now()) and returning an error if it isn’t.
| // Validate that the scheduled time is in the future | |
| if !atTime.After(time.Now()) { | |
| return cron.CronSchedule{}, fmt.Errorf("at datetime must be in the future") | |
| } |
| case "cron": | ||
| expr, ok := scheduleMap["expr"].(string) | ||
| if !ok || expr == "" { | ||
| return cron.CronSchedule{}, fmt.Errorf("expr field is required for 'cron' kind") | ||
| } | ||
| schedule.Expr = expr | ||
|
|
There was a problem hiding this comment.
For kind='cron', the expression isn’t validated at creation time. If it’s invalid, cron service computeNextRun() returns nil and the job is created enabled but will never run. Consider validating the cron expression during parseSchedule/create (e.g., ensure the next tick can be computed) and return an error on invalid expressions.
sipeed-bot
bot
added
type: enhancement
|
|
|
@envestcc Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue. |
Summary
Add a new
scheduletool that allows the AI agent to create, list, and cancel scheduled tasks through the existing cron service.Features
at- One-time task at specific datetime (e.g.,2024-01-01T12:00:00)every- Recurring task with interval in seconds (e.g., 3600 for every hour)cron- Cron expression for complex schedules (e.g.,0 9 * * *for daily at 9am)Changes
pkg/tools/schedule.go(new file, ~308 lines) - ScheduleTool implementationcmd/picoclaw/main.go- Register ScheduleTool in setupCronTool()Related Issue
Closes #351