Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmd/picoclaw/internal/gateway/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ func setupCronTool(
// Create cron service
cronService := cron.NewCronService(cronStorePath, nil)

// Apply default timezone from config for cron expressions
if cfg.Tools.Cron.DefaultTimezone != "" {
cronService.SetDefaultTimezone(cfg.Tools.Cron.DefaultTimezone)
}

// Create and register CronTool if enabled
var cronTool *tools.CronTool
if cfg.Tools.IsToolEnabled("cron") {
Expand Down
5 changes: 3 additions & 2 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,8 +578,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
ToolConfig ` envPrefix:"PICOCLAW_TOOLS_CRON_"`
ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout
DefaultTimezone string ` env:"PICOCLAW_TOOLS_CRON_DEFAULT_TIMEZONE" json:"default_timezone"`
}

type ExecConfig struct {
Expand Down
24 changes: 24 additions & 0 deletions pkg/cron/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import (
"github.com/sipeed/picoclaw/pkg/fileutil"
)

const defaultTimezone = "UTC"

type CronSchedule struct {
Kind string `json:"kind"`
AtMS *int64 `json:"atMs,omitempty"`
Expand Down Expand Up @@ -66,6 +68,7 @@ type CronService struct {
running bool
stopChan chan struct{}
gronx *gronx.Gronx
defaultTZ string // default timezone for cron expressions
}

func NewCronService(storePath string, onJob JobHandler) *CronService {
Expand Down Expand Up @@ -266,6 +269,19 @@ func (cs *CronService) computeNextRun(schedule *CronSchedule, nowMS int64) *int6

// Use gronx to calculate next run time
now := time.UnixMilli(nowMS)
// Apply timezone: schedule.TZ > service default > UTC
tz := schedule.TZ
if tz == "" {
tz = cs.defaultTZ
}
if tz == "" {
tz = defaultTimezone
}
Comment on lines +277 to +279
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default timezone literal "Asia/Shanghai" is duplicated in both the scheduling logic and method comment. Defining a single package-level constant (and reusing it in tests) would reduce the chance of docs/tests drifting from behavior if this fallback is ever changed.

Copilot uses AI. Check for mistakes.
Comment on lines +272 to +279
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description/issue mentions a 3-tier fallback ending with "Asia/Shanghai" when both schedule.TZ and config default are empty, but the implementation falls back to "UTC" (tz == "" => "UTC"). Either update the code to match the described fallback (or time.Local), or update the PR description/tests/docs to reflect that the ultimate fallback is UTC.

Copilot uses AI. Check for mistakes.
if loc, err := time.LoadLocation(tz); err == nil {
now = now.In(loc)
} else {
log.Printf("[cron] warning: failed to load timezone '%s': %v, using UTC", tz, err)
}
nextTime, err := gronx.NextTickAfter(schedule.Expr, now, false)
if err != nil {
log.Printf("[cron] failed to compute next run for expr '%s': %v", schedule.Expr, err)
Expand Down Expand Up @@ -313,6 +329,14 @@ func (cs *CronService) SetOnJob(handler JobHandler) {
cs.onJob = handler
}

// SetDefaultTimezone sets the default timezone for cron expressions.
// If empty, falls back to UTC.
func (cs *CronService) SetDefaultTimezone(tz string) {
cs.mu.Lock()
defer cs.mu.Unlock()
cs.defaultTZ = tz
}

func (cs *CronService) loadStore() error {
cs.store = &CronStore{
Version: 1,
Expand Down
147 changes: 147 additions & 0 deletions pkg/cron/timezone_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
package cron

import (
"path/filepath"
"testing"
"time"
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests rely on the host OS zoneinfo database being present (time.LoadLocation("Asia/Shanghai"/"US/Eastern")). In minimal containers or some developer environments this can be missing, causing these tests to fail even though the production logic is correct. Consider importing _ "time/tzdata" in this test file (or skipping the affected subtests when LoadLocation fails) to make the suite hermetic.

Suggested change
"time"
"time"
_ "time/tzdata"

Copilot uses AI. Check for mistakes.
)

// TestComputeNextRun_CronTimezone verifies that cron expressions respect
// the schedule.TZ field instead of always using UTC.
func TestComputeNextRun_CronTimezone(t *testing.T) {
tmpDir := t.TempDir()
storePath := filepath.Join(tmpDir, "jobs.json")
cs := NewCronService(storePath, nil)

now := time.Date(2026, 3, 4, 0, 0, 0, 0, time.UTC) // midnight UTC
nowMS := now.UnixMilli()

// Cron expr "0 9 * * *" = daily at 9:00
tests := []struct {
name string
tz string
wantHour int // expected hour in UTC of the next run
}{
{
name: "UTC timezone",
tz: "UTC",
wantHour: 9, // 9:00 UTC
},
{
name: "Asia/Shanghai timezone",
tz: "Asia/Shanghai",
wantHour: 1, // 9:00 CST = 1:00 UTC
},
{
name: "empty TZ defaults to UTC",
tz: "",
wantHour: 9, // should default to UTC
},
{
name: "US/Eastern timezone",
tz: "US/Eastern",
wantHour: 14, // 9:00 EST = 14:00 UTC (or 13:00 during DST)
},
Comment on lines +41 to +44
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test uses the legacy alias timezone ID US/Eastern. That alias isn’t guaranteed to exist across all zoneinfo distributions, which can make the test fail on some environments even though the code is correct. Prefer a canonical IANA zone like America/New_York (and then you can assert the exact expected UTC hour for the chosen date).

Copilot uses AI. Check for mistakes.
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
schedule := &CronSchedule{
Kind: "cron",
Expr: "0 9 * * *",
TZ: tt.tz,
}

nextMS := cs.computeNextRun(schedule, nowMS)
if nextMS == nil {
t.Fatal("computeNextRun returned nil")
}

nextTime := time.UnixMilli(*nextMS).UTC()

if tt.name == "US/Eastern timezone" {
// Allow for DST variation (13 or 14)
if nextTime.Hour() != 13 && nextTime.Hour() != 14 {
t.Errorf("next run hour = %d, want 13 or 14 (UTC)", nextTime.Hour())
}
Comment on lines +60 to +66
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test the reference date is 2026-03-04, when US/Eastern is not in DST, so the next run should be deterministically 14:00 UTC. Allowing 13 or 14 makes the assertion weaker and could let an off-by-one-hour bug slip through; consider asserting the exact expected hour (or computing it from the loaded location for the chosen date).

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +66
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This subtest special-cases the US/Eastern case by comparing on tt.name, which is easy to break if the display name changes. Consider branching on tt.tz (or adding a boolean in the test cases) so the behavior under test is keyed off the timezone value itself.

Copilot uses AI. Check for mistakes.
} else {
if nextTime.Hour() != tt.wantHour {
t.Errorf("next run hour = %d, want %d (UTC)", nextTime.Hour(), tt.wantHour)
}
}
})
}
}

// TestComputeNextRun_DefaultTZ_AppliesWhenSet verifies that SetDefaultTimezone
// takes effect when schedule.TZ is empty (service default overrides UTC fallback).
func TestComputeNextRun_DefaultTZ_AppliesWhenSet(t *testing.T) {
tmpDir := t.TempDir()
storePath := filepath.Join(tmpDir, "jobs.json")
cs := NewCronService(storePath, nil)

// Set a non-UTC default to verify it takes effect
cs.SetDefaultTimezone("Asia/Shanghai")

now := time.Date(2026, 3, 4, 2, 0, 0, 0, time.UTC) // 02:00 UTC = 10:00 CST
nowMS := now.UnixMilli()

scheduleEmpty := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: ""}
scheduleUTC := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: "UTC"}

nextEmpty := cs.computeNextRun(scheduleEmpty, nowMS)
nextUTC := cs.computeNextRun(scheduleUTC, nowMS)

if nextEmpty == nil || nextUTC == nil {
t.Fatal("computeNextRun returned nil")
}

// With service default Asia/Shanghai, 9:00 CST already passed (it's 10:00 CST),
// so next run should be tomorrow 9:00 CST.
// With explicit UTC, 9:00 UTC hasn't happened yet (it's 02:00 UTC).
// They must differ, proving SetDefaultTimezone takes effect.
if *nextEmpty == *nextUTC {
t.Errorf("service default TZ (Asia/Shanghai) and UTC produced same next run time")
}
}

// TestComputeNextRun_ServiceDefaultTZ verifies that SetDefaultTimezone()
// takes effect when schedule.TZ is empty (3-tier fallback).
func TestComputeNextRun_ServiceDefaultTZ(t *testing.T) {
tmpDir := t.TempDir()
storePath := filepath.Join(tmpDir, "jobs.json")
cs := NewCronService(storePath, nil)

// Set service-level default to US/Eastern
cs.SetDefaultTimezone("US/Eastern")

Comment on lines +115 to +117
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cs.SetDefaultTimezone("US/Eastern") relies on a legacy timezone alias that may not be present in all tzdata bundles. Using a canonical IANA name like America/New_York makes the test (and the intended config value) more portable.

Copilot uses AI. Check for mistakes.
now := time.Date(2026, 3, 4, 0, 0, 0, 0, time.UTC)
nowMS := now.UnixMilli()

// Schedule with empty TZ should use service default (US/Eastern), not UTC
scheduleEmpty := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: ""}
scheduleCST := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: "Asia/Shanghai"}

nextEmpty := cs.computeNextRun(scheduleEmpty, nowMS)
nextCST := cs.computeNextRun(scheduleCST, nowMS)

if nextEmpty == nil || nextCST == nil {
t.Fatal("computeNextRun returned nil")
}

// US/Eastern 9:00 != Asia/Shanghai 9:00, so they must differ
if *nextEmpty == *nextCST {
t.Errorf("service default TZ (US/Eastern) and Asia/Shanghai produced same time; SetDefaultTimezone not working")
}

// Schedule with explicit TZ should override service default
scheduleExplicit := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: "UTC"}
nextExplicit := cs.computeNextRun(scheduleExplicit, nowMS)
if nextExplicit == nil {
t.Fatal("computeNextRun returned nil")
}
nextUTC := time.UnixMilli(*nextExplicit).UTC()
if nextUTC.Hour() != 9 {
t.Errorf("explicit TZ=UTC: next run hour = %d, want 9", nextUTC.Hour())
}
}
Loading