diff --git a/go.mod b/go.mod index f60be046f1..2cfef4d5d9 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/sipeed/picoclaw -go 1.25.7 +go 1.23 require ( github.com/adhocore/gronx v1.19.6 diff --git a/pkg/config/config.go b/pkg/config/config.go index 76d312daec..e15c37d29c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -648,7 +648,9 @@ type WebToolsConfig struct { type CronToolsConfig struct { ToolConfig ` envPrefix:"PICOCLAW_TOOLS_CRON_"` - ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout + ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout + AllowCommand bool ` env:"PICOCLAW_TOOLS_CRON_ALLOW_COMMAND" json:"allow_command"` + MinIntervalSeconds int ` env:"PICOCLAW_TOOLS_CRON_MIN_INTERVAL_SECONDS" json:"min_interval_seconds"` // 0 means disable check, default 60 } type ExecConfig struct { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 0892d45f4a..daddcd4864 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -413,6 +413,8 @@ func DefaultConfig() *Config { Enabled: true, }, ExecTimeoutMinutes: 5, + AllowCommand: true, + MinIntervalSeconds: 60, }, Exec: ExecConfig{ ToolConfig: ToolConfig{ diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 6af0aa9e14..e3635ddb14 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -6,6 +6,8 @@ import ( "strings" "time" + "github.com/adhocore/gronx" + "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/cron" @@ -19,29 +21,40 @@ type JobExecutor interface { // CronTool provides scheduling capabilities for the agent type CronTool struct { - cronService *cron.CronService - executor JobExecutor - msgBus *bus.MessageBus - execTool *ExecTool + cronService *cron.CronService + executor JobExecutor + msgBus *bus.MessageBus + execTool *ExecTool + minIntervalSeconds int } // NewCronTool creates a new CronTool // execTimeout: 0 means no timeout, >0 sets the timeout duration func NewCronTool( cronService *cron.CronService, executor JobExecutor, msgBus *bus.MessageBus, workspace string, restrict bool, - execTimeout time.Duration, config *config.Config, + execTimeout time.Duration, cfg *config.Config, ) (*CronTool, error) { - execTool, err := NewExecToolWithConfig(workspace, restrict, config) + execTool, err := NewExecToolWithConfig(workspace, restrict, cfg) if err != nil { return nil, fmt.Errorf("unable to configure exec tool: %w", err) } execTool.SetTimeout(execTimeout) + + minIntervalSeconds := 60 // default 1 minute + if cfg != nil { + minIntervalSeconds = cfg.Tools.Cron.MinIntervalSeconds + if minIntervalSeconds < 0 { + minIntervalSeconds = 60 + } + } + return &CronTool{ - cronService: cronService, - executor: executor, - msgBus: msgBus, - execTool: execTool, + cronService: cronService, + executor: executor, + msgBus: msgBus, + execTool: execTool, + minIntervalSeconds: minIntervalSeconds, }, nil } @@ -121,6 +134,41 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *ToolResult } } +// validateMinInterval checks if the schedule interval meets the minimum requirement +func (t *CronTool) validateMinInterval(schedule cron.CronSchedule) error { + if t.minIntervalSeconds <= 0 { + return nil + } + + switch schedule.Kind { + case "every": + if schedule.EveryMS != nil { + intervalSeconds := *schedule.EveryMS / 1000 + if intervalSeconds < int64(t.minIntervalSeconds) { + return fmt.Errorf("interval too short: minimum allowed is %d seconds, got %d seconds", t.minIntervalSeconds, intervalSeconds) + } + } + case "cron": + if schedule.Expr != "" { + // Calculate the interval between next two ticks + now := time.Now() + next1, err := gronx.NextTickAfter(schedule.Expr, now, false) + if err != nil { + return fmt.Errorf("invalid cron expression: %w", err) + } + next2, err := gronx.NextTickAfter(schedule.Expr, next1, false) + if err != nil { + return fmt.Errorf("invalid cron expression: %w", err) + } + intervalSeconds := int64(next2.Sub(next1).Seconds()) + if intervalSeconds < int64(t.minIntervalSeconds) { + return fmt.Errorf("cron interval too short: minimum allowed is %d seconds, calculated interval is %d seconds", t.minIntervalSeconds, intervalSeconds) + } + } + } + return nil +} + func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult { channel := ToolChannel(ctx) chatID := ToolChatID(ctx) @@ -184,6 +232,13 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult deliver = false } + // Validate minimum interval for recurring schedules + if t.minIntervalSeconds > 0 { + if err := t.validateMinInterval(schedule); err != nil { + return ErrorResult(err.Error()) + } + } + // Truncate message for job name (max 30 chars) messagePreview := utils.Truncate(message, 30) diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go new file mode 100644 index 0000000000..53758c0b47 --- /dev/null +++ b/pkg/tools/cron_test.go @@ -0,0 +1,185 @@ +package tools + +import ( + "context" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/cron" +) + +func newTestCronToolWithConfig(t *testing.T, cfg *config.Config) *CronTool { + t.Helper() + storePath := filepath.Join(t.TempDir(), "cron.json") + cronService := cron.NewCronService(storePath, nil) + msgBus := bus.NewMessageBus() + tool, err := NewCronTool(cronService, nil, msgBus, t.TempDir(), true, 0, cfg) + if err != nil { + t.Fatalf("NewCronTool() error: %v", err) + } + return tool +} + +func newTestCronTool(t *testing.T) *CronTool { + t.Helper() + return newTestCronToolWithConfig(t, config.DefaultConfig()) +} + +// TestCronTool_MinIntervalSeconds validates that min_interval_seconds config prevents excessive scheduling +func TestCronTool_MinIntervalSeconds(t *testing.T) { + // Test with default config (min_interval_seconds = 60) + tool := newTestCronTool(t) + ctx := WithToolContext(context.Background(), "cli", "direct") + + // Test every_seconds below minimum (should fail) + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "every_seconds": float64(30), // 30 seconds, below default 60 + }) + + if !result.IsError { + t.Fatal("expected error for interval below minimum") + } + if !strings.Contains(result.ForLLM, "interval too short") { + t.Errorf("expected 'interval too short' error, got: %s", result.ForLLM) + } + + // Test every_seconds at minimum (should succeed) + result = tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "every_seconds": float64(60), // 60 seconds, at default minimum + }) + + if result.IsError { + t.Fatalf("expected success for interval at minimum, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinIntervalSecondsDisabled validates that min_interval_seconds = 0 disables the check +func TestCronTool_MinIntervalSecondsDisabled(t *testing.T) { + // Create config with min_interval_seconds = 0 (disabled) + cfg := config.DefaultConfig() + cfg.Tools.Cron.MinIntervalSeconds = 0 + + tool := newTestCronToolWithConfig(t, cfg) + ctx := WithToolContext(context.Background(), "cli", "direct") + + // Test every_seconds below default minimum (should succeed when check is disabled) + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "every_seconds": float64(10), // 10 seconds, would fail if check was enabled + }) + + if result.IsError { + t.Fatalf("expected success when min_interval_seconds is disabled, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinIntervalSecondsCron validates cron expression interval checking +func TestCronTool_MinIntervalSecondsCron(t *testing.T) { + // Test with default config (min_interval_seconds = 60) + tool := newTestCronTool(t) + ctx := WithToolContext(context.Background(), "cli", "direct") + + // Test cron expression with interval below minimum (every 30 seconds) + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "cron_expr": "*/30 * * * * *", // every 30 seconds + }) + + if !result.IsError { + t.Fatal("expected error for cron interval below minimum") + } + if !strings.Contains(result.ForLLM, "cron interval too short") { + t.Errorf("expected 'cron interval too short' error, got: %s", result.ForLLM) + } + + // Test cron expression with interval at minimum (every minute) + result = tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "cron_expr": "0 * * * * *", // every minute + }) + + if result.IsError { + t.Fatalf("expected success for cron interval at minimum, got: %s", result.ForLLM) + } +} + +// TestCronTool_AtSecondsNotAffected validates that at_seconds is not affected by min_interval_seconds +func TestCronTool_AtSecondsNotAffected(t *testing.T) { + tool := newTestCronTool(t) + ctx := WithToolContext(context.Background(), "cli", "direct") + + // Test at_seconds (one-time) should not be affected by min_interval_seconds + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "at_seconds": float64(10), // 10 seconds from now + }) + + if result.IsError { + t.Fatalf("expected success for at_seconds regardless of min_interval_seconds, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinIntervalSecondsCustom validates custom min_interval_seconds values +func TestCronTool_MinIntervalSecondsCustom(t *testing.T) { + // Create config with custom min_interval_seconds = 120 + cfg := config.DefaultConfig() + cfg.Tools.Cron.MinIntervalSeconds = 120 + + tool := newTestCronToolWithConfig(t, cfg) + ctx := WithToolContext(context.Background(), "cli", "direct") + + // Test every_seconds below custom minimum (should fail) + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "every_seconds": float64(90), // 90 seconds, below custom minimum 120 + }) + + if !result.IsError { + t.Fatal("expected error for interval below custom minimum") + } + + // Test every_seconds at custom minimum (should succeed) + result = tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "every_seconds": float64(120), // 120 seconds, at custom minimum + }) + + if result.IsError { + t.Fatalf("expected success for interval at custom minimum, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinIntervalSecondsNegative validates that negative values are treated as default +func TestCronTool_MinIntervalSecondsNegative(t *testing.T) { + // Create config with negative min_interval_seconds (should use default 60) + cfg := config.DefaultConfig() + cfg.Tools.Cron.MinIntervalSeconds = -1 + + tool := newTestCronToolWithConfig(t, cfg) + ctx := WithToolContext(context.Background(), "cli", "direct") + + // Test every_seconds below default minimum (should fail with default 60) + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "test reminder", + "every_seconds": float64(30), // 30 seconds, below default 60 + }) + + if !result.IsError { + t.Fatal("expected error for interval below default minimum when negative value provided") + } +}