From 2ddc53314f1f31fca323b79fddd8633f6f88df79 Mon Sep 17 00:00:00 2001 From: envestcc Date: Fri, 20 Feb 2026 19:23:57 +0800 Subject: [PATCH] fix: address review comments --- pkg/tools/schedule.go | 42 ++- pkg/tools/schedule_test.go | 585 +++++++++++++++++++++++++++++++++++++ 2 files changed, 613 insertions(+), 14 deletions(-) create mode 100644 pkg/tools/schedule_test.go diff --git a/pkg/tools/schedule.go b/pkg/tools/schedule.go index a3dd7775e..93528902d 100644 --- a/pkg/tools/schedule.go +++ b/pkg/tools/schedule.go @@ -68,7 +68,7 @@ func (t *ScheduleTool) Parameters() map[string]interface{} { }, "every_seconds": map[string]interface{}{ "type": "integer", - "description": "Interval in seconds for 'every' kind. Example: 3600 for every hour.", + "description": "Interval in seconds for 'every' kind. Example: 3600 for every hour. Must be a positive integer.", }, "expr": map[string]interface{}{ "type": "string", @@ -76,7 +76,7 @@ func (t *ScheduleTool) Parameters() map[string]interface{} { }, "timezone": map[string]interface{}{ "type": "string", - "description": "Timezone for the schedule (e.g., 'Asia/Shanghai', 'UTC'). If not specified, uses system timezone.", + "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.", }, }, "required": []string{"kind"}, @@ -202,22 +202,32 @@ func (t *ScheduleTool) parseSchedule(scheduleMap map[string]interface{}) (cron.C return cron.CronSchedule{}, fmt.Errorf("at field is required for 'at' kind") } - // 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 + // Determine location to use for parsing times without explicit offset + var loc *time.Location if tz != "" { - loc, err := time.LoadLocation(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 (with explicit offset) + 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 only for offset-aware inputs (naive inputs already parsed in loc) + if tz != "" && !usedNaiveLayout { atTime = atTime.In(loc) } @@ -232,6 +242,10 @@ func (t *ScheduleTool) parseSchedule(scheduleMap map[string]interface{}) (cron.C if everySeconds <= 0 { return cron.CronSchedule{}, fmt.Errorf("every_seconds must be positive") } + // Reject non-integer values to avoid surprising schedules + if everySeconds != float64(int64(everySeconds)) { + return cron.CronSchedule{}, fmt.Errorf("every_seconds must be an integer value, got %.2f", everySeconds) + } everyMS := int64(everySeconds) * 1000 schedule.EveryMS = &everyMS diff --git a/pkg/tools/schedule_test.go b/pkg/tools/schedule_test.go new file mode 100644 index 000000000..cb691135a --- /dev/null +++ b/pkg/tools/schedule_test.go @@ -0,0 +1,585 @@ +package tools + +import ( + "context" + "testing" + "time" + + "github.com/sipeed/picoclaw/pkg/cron" +) + +func TestScheduleTool_Name(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + + if st.Name() != "schedule" { + t.Errorf("expected name 'schedule', got '%s'", st.Name()) + } +} + +func TestScheduleTool_Description(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + + desc := st.Description() + if desc == "" { + t.Error("description should not be empty") + } +} + +func TestScheduleTool_Parameters(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + + params := st.Parameters() + if params["type"] != "object" { + t.Error("parameters should be type object") + } + + props, ok := params["properties"].(map[string]interface{}) + if !ok { + t.Fatal("properties should be a map") + } + + // Check required fields + required, ok := params["required"].([]string) + if !ok || len(required) != 1 || required[0] != "action" { + t.Error("action should be required") + } + + // Check schedule properties + schedule, ok := props["schedule"].(map[string]interface{}) + if !ok { + t.Fatal("schedule should be a map") + } + + scheduleProps, ok := schedule["properties"].(map[string]interface{}) + if !ok { + t.Fatal("schedule properties should be a map") + } + + // Verify kind, at, every_seconds, expr, timezone exist + for _, key := range []string{"kind", "at", "every_seconds", "expr", "timezone"} { + if _, ok := scheduleProps[key]; !ok { + t.Errorf("schedule.%s should exist", key) + } + } +} + +func TestScheduleTool_SetContext(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + + st.SetContext("test-channel", "test-chat-id") + + // Can't directly access private fields, but we can verify it doesn't panic + st.SetContext("another-channel", "another-chat-id") +} + +func TestScheduleTool_Execute_InvalidAction(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{"action": "invalid"}) + if !result.IsError { + t.Error("invalid action should return error") + } + + result = st.Execute(ctx, map[string]interface{}{}) + if !result.IsError { + t.Error("missing action should return error") + } +} + +func TestScheduleTool_Execute_ListEmpty(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{"action": "list"}) + if result.IsError { + t.Errorf("list should not error: %s", result.ForLLM) + } + if result.ForLLM != "No scheduled jobs" { + t.Errorf("expected 'No scheduled jobs', got '%s'", result.ForLLM) + } +} + +func TestScheduleTool_Execute_Create_NoContext(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(60), + }, + }) + + if !result.IsError { + t.Error("create without context should return error") + } +} + +func TestScheduleTool_Execute_Create_MissingName(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(60), + }, + }) + + if !result.IsError { + t.Error("create without name should return error") + } +} + +func TestScheduleTool_Execute_Create_MissingMessage(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(60), + }, + }) + + if !result.IsError { + t.Error("create without message should return error") + } +} + +func TestScheduleTool_Execute_Create_MissingSchedule(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + }) + + if !result.IsError { + t.Error("create without schedule should return error") + } +} + +func TestScheduleTool_Execute_Create_Every(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Every Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(60), + }, + }) + + if result.IsError { + t.Errorf("create every job should succeed: %s", result.ForLLM) + } + + // Verify job was created + jobs := cs.ListJobs(true) + if len(jobs) != 1 { + t.Errorf("expected 1 job, got %d", len(jobs)) + } + if jobs[0].Name != "Test Every Job" { + t.Errorf("expected job name 'Test Every Job', got '%s'", jobs[0].Name) + } +} + +func TestScheduleTool_Execute_Create_Cron(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Daily Job", + "message": "Daily message", + "schedule": map[string]interface{}{ + "kind": "cron", + "expr": "0 9 * * *", + }, + }) + + if result.IsError { + t.Errorf("create cron job should succeed: %s", result.ForLLM) + } + + jobs := cs.ListJobs(true) + if len(jobs) != 1 { + t.Errorf("expected 1 job, got %d", len(jobs)) + } +} + +func TestScheduleTool_Execute_Create_At(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + // Use a future time + futureTime := time.Now().Add(1 * time.Hour).Format("2006-01-02T15:04:05") + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "One-time Job", + "message": "One-time message", + "schedule": map[string]interface{}{ + "kind": "at", + "at": futureTime, + }, + }) + + if result.IsError { + t.Errorf("create at job should succeed: %s", result.ForLLM) + } + + jobs := cs.ListJobs(true) + if len(jobs) != 1 { + t.Errorf("expected 1 job, got %d", len(jobs)) + } + if jobs[0].Schedule.Kind != "at" { + t.Errorf("expected schedule kind 'at', got '%s'", jobs[0].Schedule.Kind) + } +} + +func TestScheduleTool_Execute_Create_At_WithTimezone(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + // Use a naive datetime with timezone + futureTime := time.Now().Add(1 * time.Hour).Format("2006-01-02T15:04:05") + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "TZ Job", + "message": "TZ message", + "schedule": map[string]interface{}{ + "kind": "at", + "at": futureTime, + "timezone": "UTC", + }, + }) + + if result.IsError { + t.Errorf("create at job with timezone should succeed: %s", result.ForLLM) + } + + jobs := cs.ListJobs(true) + if len(jobs) != 1 { + t.Errorf("expected 1 job, got %d", len(jobs)) + } + if jobs[0].Schedule.TZ != "UTC" { + t.Errorf("expected TZ 'UTC', got '%s'", jobs[0].Schedule.TZ) + } +} + +func TestScheduleTool_Execute_Create_Every_NonInteger(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(60.5), + }, + }) + + if !result.IsError { + t.Error("create with non-integer every_seconds should return error") + } +} + +func TestScheduleTool_Execute_Create_Every_Negative(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(-60), + }, + }) + + if !result.IsError { + t.Error("create with negative every_seconds should return error") + } +} + +func TestScheduleTool_Execute_Create_InvalidKind(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "invalid", + }, + }) + + if !result.IsError { + t.Error("create with invalid kind should return error") + } +} + +func TestScheduleTool_Execute_Create_At_InvalidFormat(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "at", + "at": "invalid-date", + }, + }) + + if !result.IsError { + t.Error("create with invalid date format should return error") + } +} + +func TestScheduleTool_Execute_Create_At_MissingAt(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "at", + }, + }) + + if !result.IsError { + t.Error("create at job without at field should return error") + } +} + +func TestScheduleTool_Execute_Create_Every_MissingSeconds(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "every", + }, + }) + + if !result.IsError { + t.Error("create every job without every_seconds should return error") + } +} + +func TestScheduleTool_Execute_Create_Cron_MissingExpr(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "cron", + }, + }) + + if !result.IsError { + t.Error("create cron job without expr should return error") + } +} + +func TestScheduleTool_Execute_Create_InvalidTimezone(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + futureTime := time.Now().Add(1 * time.Hour).Format("2006-01-02T15:04:05") + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "at", + "at": futureTime, + "timezone": "Invalid/Timezone", + }, + }) + + if !result.IsError { + t.Error("create with invalid timezone should return error") + } +} + +func TestScheduleTool_Execute_ListWithJobs(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + // Create a job first + _, err := cs.AddJob("Test Job", cron.CronSchedule{ + Kind: "every", + EveryMS: func() *int64 { v := int64(60000); return &v }(), + }, "Test message", true, "test-channel", "test-chat-id") + if err != nil { + t.Fatalf("failed to create test job: %v", err) + } + + result := st.Execute(ctx, map[string]interface{}{"action": "list"}) + if result.IsError { + t.Errorf("list should not error: %s", result.ForLLM) + } + if result.ForLLM == "No scheduled jobs" { + t.Error("should show jobs, not 'No scheduled jobs'") + } +} + +func TestScheduleTool_Execute_Cancel_MissingID(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "cancel", + }) + + if !result.IsError { + t.Error("cancel without id should return error") + } +} + +func TestScheduleTool_Execute_Cancel_NotFound(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "cancel", + "id": "nonexistent-id", + }) + + if !result.IsError { + t.Error("cancel with non-existent id should return error") + } +} + +func TestScheduleTool_Execute_Cancel_Success(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + // Create a job first + job, err := cs.AddJob("Test Job", cron.CronSchedule{ + Kind: "every", + EveryMS: func() *int64 { v := int64(60000); return &v }(), + }, "Test message", true, "test-channel", "test-chat-id") + if err != nil { + t.Fatalf("failed to create test job: %v", err) + } + + result := st.Execute(ctx, map[string]interface{}{ + "action": "cancel", + "id": job.ID, + }) + + if result.IsError { + t.Errorf("cancel should succeed: %s", result.ForLLM) + } + + // Verify job was removed + jobs := cs.ListJobs(true) + if len(jobs) != 0 { + t.Errorf("expected 0 jobs after cancel, got %d", len(jobs)) + } +} + +func TestScheduleTool_Execute_DeliverFalse(t *testing.T) { + cs := cron.NewCronService(t.TempDir()+"/test.json", nil) + st := NewScheduleTool(cs) + st.SetContext("test-channel", "test-chat-id") + ctx := context.Background() + + result := st.Execute(ctx, map[string]interface{}{ + "action": "create", + "name": "Test Job", + "message": "Test message", + "schedule": map[string]interface{}{ + "kind": "every", + "every_seconds": float64(60), + }, + "deliver": false, + }) + + if result.IsError { + t.Errorf("create with deliver=false should succeed: %s", result.ForLLM) + } + + jobs := cs.ListJobs(true) + if len(jobs) != 1 { + t.Errorf("expected 1 job, got %d", len(jobs)) + } + if jobs[0].Payload.Deliver { + t.Error("expected Deliver to be false") + } +}