From af12798960e3de5aa10173c5949ea462037dcc3c Mon Sep 17 00:00:00 2001 From: fernando stahelin Date: Mon, 16 Mar 2026 18:26:23 -0300 Subject: [PATCH] feat(cron): add min_interval_seconds to prevent excessive scheduling Add a configurable minimum interval for recurring cron jobs to prevent token waste and API abuse. The check applies to both 'every' and 'cron' schedule kinds; one-time 'at' jobs are unaffected. Default: 60 seconds. Set to 0 to disable. Fixes #1655 Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/tools_configuration.md | 8 +-- pkg/config/config.go | 3 +- pkg/config/defaults.go | 1 + pkg/cron/service.go | 5 ++ pkg/tools/cron.go | 61 ++++++++++++++++++--- pkg/tools/cron_test.go | 104 ++++++++++++++++++++++++++++++++++++ 6 files changed, 170 insertions(+), 12 deletions(-) diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index 8c8eb31f0..51462300b 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -104,9 +104,10 @@ By default, PicoClaw blocks the following dangerous commands: The cron tool is used for scheduling periodic tasks. -| Config | Type | Default | Description | -|------------------------|------|---------|------------------------------------------------| -| `exec_timeout_minutes` | int | 5 | Execution timeout in minutes, 0 means no limit | +| Config | Type | Default | Description | +|------------------------|------|---------|--------------------------------------------------------------| +| `exec_timeout_minutes` | int | 5 | Execution timeout in minutes, 0 means no limit | +| `min_interval_seconds` | int | 60 | Minimum allowed interval for recurring jobs, 0 means no limit | ## MCP Tool @@ -311,6 +312,7 @@ For example: - `PICOCLAW_TOOLS_WEB_BRAVE_ENABLED=true` - `PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS=false` - `PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES=10` +- `PICOCLAW_TOOLS_CRON_MIN_INTERVAL_SECONDS=60` - `PICOCLAW_TOOLS_MCP_ENABLED=true` Note: Nested map-style config (for example `tools.mcp.servers..*`) is configured in `config.json` rather than diff --git a/pkg/config/config.go b/pkg/config/config.go index 2937c36e4..e7e21c001 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -700,7 +700,8 @@ 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 + MinIntervalSeconds int ` env:"PICOCLAW_TOOLS_CRON_MIN_INTERVAL_SECONDS" json:"min_interval_seconds"` // minimum interval for recurring jobs; 0 means no limit } type ExecConfig struct { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index dc534d852..ff32bd509 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -452,6 +452,7 @@ func DefaultConfig() *Config { Enabled: true, }, ExecTimeoutMinutes: 5, + MinIntervalSeconds: 60, }, Exec: ExecConfig{ ToolConfig: ToolConfig{ diff --git a/pkg/cron/service.go b/pkg/cron/service.go index 04775ac42..6f6e56101 100644 --- a/pkg/cron/service.go +++ b/pkg/cron/service.go @@ -506,6 +506,11 @@ func (cs *CronService) Status() map[string]any { } } +// NextTickAfter returns the next time a cron expression fires after the given reference time. +func NextTickAfter(expr string, after time.Time) (time.Time, error) { + return gronx.NextTickAfter(expr, after, false) +} + func generateID() string { // Use crypto/rand for better uniqueness under concurrent access b := make([]byte, 8) diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 25608a54c..1fb77751c 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -20,10 +20,11 @@ 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 @@ -39,10 +40,11 @@ func NewCronTool( execTool.SetTimeout(execTimeout) return &CronTool{ - cronService: cronService, - executor: executor, - msgBus: msgBus, - execTool: execTool, + cronService: cronService, + executor: executor, + msgBus: msgBus, + execTool: execTool, + minIntervalSeconds: config.Tools.Cron.MinIntervalSeconds, }, nil } @@ -174,6 +176,13 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult return ErrorResult("one of at_seconds, every_seconds, or cron_expr is required") } + // Enforce minimum interval for recurring schedules + if t.minIntervalSeconds > 0 { + if err := t.validateMinInterval(schedule); err != nil { + return ErrorResult(err.Error()) + } + } + // Read deliver parameter, default to false so scheduled tasks execute through the agent deliver := false if d, ok := args["deliver"].(bool); ok { @@ -274,6 +283,42 @@ func (t *CronTool) enableJob(args map[string]any, enable bool) *ToolResult { return SilentResult(fmt.Sprintf("Cron job '%s' %s", job.Name, status)) } +// validateMinInterval checks that the schedule interval is not below the configured minimum. +// It validates "every" schedules directly and estimates the interval for cron expressions. +func (t *CronTool) validateMinInterval(schedule cron.CronSchedule) error { + minMS := int64(t.minIntervalSeconds) * 1000 + + switch schedule.Kind { + case "every": + if schedule.EveryMS != nil && *schedule.EveryMS < minMS { + return fmt.Errorf( + "interval %ds is below the minimum allowed interval of %ds", + *schedule.EveryMS/1000, t.minIntervalSeconds, + ) + } + case "cron": + if schedule.Expr != "" { + now := time.Now() + next1, err := cron.NextTickAfter(schedule.Expr, now) + if err != nil { + return nil // let gronx validate later + } + next2, err := cron.NextTickAfter(schedule.Expr, next1) + if err != nil { + return nil + } + gapMS := next2.Sub(next1).Milliseconds() + if gapMS < minMS { + return fmt.Errorf( + "cron expression '%s' fires every %ds, which is below the minimum allowed interval of %ds", + schedule.Expr, gapMS/1000, t.minIntervalSeconds, + ) + } + } + } + return nil +} + // ExecuteJob executes a cron job through the agent func (t *CronTool) ExecuteJob(ctx context.Context, job *cron.CronJob) string { // Get channel/chatID from job payload diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go index f1e857949..6f55572b9 100644 --- a/pkg/tools/cron_test.go +++ b/pkg/tools/cron_test.go @@ -12,11 +12,17 @@ import ( ) func newTestCronTool(t *testing.T) *CronTool { + t.Helper() + return newTestCronToolWithMinInterval(t, 0) +} + +func newTestCronToolWithMinInterval(t *testing.T, minInterval int) *CronTool { t.Helper() storePath := filepath.Join(t.TempDir(), "cron.json") cronService := cron.NewCronService(storePath, nil) msgBus := bus.NewMessageBus() cfg := config.DefaultConfig() + cfg.Tools.Cron.MinIntervalSeconds = minInterval tool, err := NewCronTool(cronService, nil, msgBus, t.TempDir(), true, 0, cfg) if err != nil { t.Fatalf("NewCronTool() error: %v", err) @@ -136,3 +142,101 @@ func TestCronTool_NonCommandJobDefaultsDeliverToFalse(t *testing.T) { t.Fatal("expected deliver=false by default for non-command jobs") } } + +// TestCronTool_MinInterval_EveryBelowLimit verifies that every_seconds below min_interval is rejected +func TestCronTool_MinInterval_EveryBelowLimit(t *testing.T) { + tool := newTestCronToolWithMinInterval(t, 60) + ctx := WithToolContext(context.Background(), "cli", "direct") + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "too fast", + "every_seconds": float64(5), + }) + + if !result.IsError { + t.Fatal("expected error for every_seconds below min_interval") + } + if !strings.Contains(result.ForLLM, "below the minimum allowed interval") { + t.Errorf("expected min interval error, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinInterval_EveryAboveLimit verifies that every_seconds at or above min_interval is accepted +func TestCronTool_MinInterval_EveryAboveLimit(t *testing.T) { + tool := newTestCronToolWithMinInterval(t, 60) + ctx := WithToolContext(context.Background(), "cli", "direct") + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "fast enough", + "every_seconds": float64(60), + }) + + if result.IsError { + t.Fatalf("expected every_seconds at min_interval to succeed, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinInterval_CronExprBelowLimit verifies that cron expressions firing too frequently are rejected +func TestCronTool_MinInterval_CronExprBelowLimit(t *testing.T) { + tool := newTestCronToolWithMinInterval(t, 120) + ctx := WithToolContext(context.Background(), "cli", "direct") + // "* * * * *" fires every minute (60s), which is below 120s min + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "every minute", + "cron_expr": "* * * * *", + }) + + if !result.IsError { + t.Fatal("expected error for cron expression below min_interval") + } + if !strings.Contains(result.ForLLM, "below the minimum allowed interval") { + t.Errorf("expected min interval error, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinInterval_CronExprAboveLimit verifies that cron expressions with sufficient intervals are accepted +func TestCronTool_MinInterval_CronExprAboveLimit(t *testing.T) { + tool := newTestCronToolWithMinInterval(t, 60) + ctx := WithToolContext(context.Background(), "cli", "direct") + // "0 * * * *" fires every hour (3600s), well above 60s min + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "every hour", + "cron_expr": "0 * * * *", + }) + + if result.IsError { + t.Fatalf("expected cron expression above min_interval to succeed, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinInterval_AtJobNotAffected verifies that one-time (at) jobs are not affected by min_interval +func TestCronTool_MinInterval_AtJobNotAffected(t *testing.T) { + tool := newTestCronToolWithMinInterval(t, 3600) + ctx := WithToolContext(context.Background(), "cli", "direct") + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "one time only", + "at_seconds": float64(5), + }) + + if result.IsError { + t.Fatalf("expected one-time at_seconds job to bypass min_interval, got: %s", result.ForLLM) + } +} + +// TestCronTool_MinInterval_ZeroDisablesCheck verifies that min_interval=0 disables the check +func TestCronTool_MinInterval_ZeroDisablesCheck(t *testing.T) { + tool := newTestCronToolWithMinInterval(t, 0) + ctx := WithToolContext(context.Background(), "cli", "direct") + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "very fast", + "every_seconds": float64(1), + }) + + if result.IsError { + t.Fatalf("expected every_seconds=1 to succeed when min_interval=0, got: %s", result.ForLLM) + } +}