fix(cron): apply timezone from schedule.TZ for cron expressions by AaronJny · Pull Request #1046 · sipeed/picoclaw

AaronJny · 2026-03-04T01:53:05Z

📝 Description

Fix cron expressions always being evaluated in UTC, ignoring the schedule.TZ
field. Add a configurable default_timezone in tools.cron config.

Timezone resolution uses a 3-tier fallback:

Job-specific: schedule.TZ from individual cron job
Server-wide: tools.cron.default_timezone from config
Hardcoded: UTC as final fallback

Invalid timezone values are logged with a warning and fall back to UTC.

🗣️ Type of Change

🐞 Bug fix (non-breaking change which fixes an issue)

🤖 AI Code Generation

🛠️ Mostly AI-generated (AI draft, Human verified/modified)

🔗 Related Issue

Fixes #1044

📚 Technical Context (Skip for Docs)

Root cause: computeNextRun used time.UnixMilli() which returns UTC.
Cron expressions like 0 9 * * * were evaluated at UTC 9:00 instead of
local 9:00.
Fix: Convert reference time to the target timezone before passing to
gronx.NextTickAfter.

🧪 Test Environment

Hardware: Linux server (runtime), MacBook Apple Silicon (development)
OS: Ubuntu Linux (runtime), macOS (development & testing)
Model/Provider: GPT-5.2, GPT-5.3-codex
Channels: Discord

☑️ Checklist

My code/docs follow the style of this project.
I have performed a self-review of my own changes.
I have updated the documentation accordingly.

Copilot

Pull request overview

Fixes cron schedules being evaluated in UTC by applying a resolved timezone (per-schedule TZ, then server config default, then a hard fallback) before calling gronx.NextTickAfter().

Changes:

Apply timezone conversion in CronService.computeNextRun() for Kind == "cron" using schedule.TZ with fallbacks.
Add tools.cron.default_timezone to config and wire it into gateway cron tool setup.
Add unit tests covering timezone behavior and the 3-tier fallback.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`pkg/cron/service.go`	Applies timezone selection/conversion for cron next-run calculation; adds `defaultTZ` + setter.
`pkg/config/config.go`	Adds `tools.cron.default_timezone` config field (JSON + env).
`cmd/picoclaw/internal/gateway/helpers.go`	Sets cron service default timezone from loaded config.
`pkg/cron/timezone_test.go`	Adds tests asserting cron timezone evaluation and fallback behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-04T01:56:38Z

pkg/cron/timezone_test.go

+			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())
+				}


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 · 2026-03-04T01:56:38Z

pkg/cron/service.go

+		// Apply timezone: schedule.TZ > service default > "Asia/Shanghai"
+		tz := schedule.TZ
+		if tz == "" {
+			tz = cs.defaultTZ
+		}
+		if tz == "" {
+			tz = "Asia/Shanghai"
+		}
+		if loc, err := time.LoadLocation(tz); err == nil {
+			now = now.In(loc)
+		}


The 3-tier fallback is not applied when time.LoadLocation(tz) fails (e.g., an invalid/LLM-generated schedule.TZ like "UTC+8" or extra whitespace). In that case now stays in UTC and the job silently schedules in UTC rather than falling back to cs.defaultTZ / "Asia/Shanghai". Consider trimming TZ strings and, on LoadLocation error, trying the next fallback(s) and emitting a warning log so misconfigurations don’t silently change scheduling semantics.

Copilot · 2026-03-04T01:56:38Z

pkg/cron/service.go

+		if tz == "" {
+			tz = "Asia/Shanghai"
+		}


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.

yinwm

Code Review Summary

This PR correctly fixes the timezone issue in cron expressions. The 3-tier fallback design is sensible, and test coverage is solid.

✅ What's Good

Accurate problem identification - Correctly identified that time.UnixMilli() returns UTC time
3-tier fallback design - schedule.TZ → config default → "Asia/Shanghai" is logical
Comprehensive tests - New test file covers timezone scenarios and fallback logic well
Thread safety - SetDefaultTimezone() uses proper locking, and all computeNextRun callers hold locks

⚠️ Suggestions

1. Silent fallback on invalid timezone (Medium Priority)

if loc, err := time.LoadLocation(tz); err == nil {
    now = now.In(loc)
}

When time.LoadLocation fails (e.g., invalid timezone like "Invalid/Zone"), the code silently continues with UTC. Consider adding a warning log:

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)
}

This helps users debug misconfiguration.

2. Default timezone choice for international projects

For open-source projects targeting a global audience, UTC is the industry standard default (used by Kubernetes CronJob, AWS EventBridge, GitHub Actions, etc.):

No daylight saving time complications
Globally neutral
Consistent behavior regardless of server location

Consider changing the hard-coded default from "Asia/Shanghai" to "UTC", or at least document this design decision clearly.

📝 Verdict

Approve with minor suggestions. The core logic is correct and well-tested. The suggestions above are improvements, not blockers.

Nice work! 🎉

AaronJny · 2026-03-04T12:04:05Z

Thank you for the thorough review! Both suggestions have been addressed in 3f4e461:

Warning log for invalid timezone — Added log.Printf("[cron] warning: ...") in the else branch of time.LoadLocation.
Default timezone changed to UTC — Replaced "Asia/Shanghai" with "UTC" as the hard-coded fallback. Updated all comments and tests accordingly.

All 6 timezone tests pass ✅

CronSchedule has a TZ field but computeNextRun() ignores it, using time.UnixMilli() which returns UTC. This causes cron expressions like '0 9 * * *' (daily at 9am) to trigger at UTC 9:00 = CST 17:00. Now loads schedule.TZ (defaulting to Asia/Shanghai when empty) and converts the reference time to the target timezone before computing the next tick with gronx.

Refactor the hardcoded 'Asia/Shanghai' default timezone to be configurable. Timezone resolution follows a 3-tier fallback: 1. schedule.TZ (per-job, set by LLM via cron_expr) 2. tools.cron.default_timezone in config.json (server-wide) 3. 'Asia/Shanghai' as ultimate fallback Changes: - config: add DefaultTimezone field to CronToolsConfig - cron: add SetDefaultTimezone() method to CronService - cron: update computeNextRun() to use 3-tier fallback - gateway: wire config.Tools.Cron.DefaultTimezone to CronService - test: add TestComputeNextRun_ServiceDefaultTZ to verify 3-tier Usage in config.json: {"tools": {"cron": {"default_timezone": "US/Eastern"}}}

Address reviewer feedback (yinwm): - Change hard-coded default from Asia/Shanghai to UTC, which is the industry standard for open-source projects (Kubernetes, AWS, etc.) - Add warning log when time.LoadLocation fails for a configured timezone to help users debug misconfiguration - Update tests to reflect UTC default

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T08:14:46Z

pkg/cron/service.go

+		// Apply timezone: schedule.TZ > service default > UTC
+		tz := schedule.TZ
+		if tz == "" {
+			tz = cs.defaultTZ
+		}
+		if tz == "" {
+			tz = "UTC"
+		}


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 · 2026-03-05T08:14:46Z

pkg/cron/timezone_test.go

+// TestComputeNextRun_CronTimezone verifies Patch #4: cron expressions should
+// respect the schedule.TZ field instead of always using UTC.


Test comment refers to "Patch #4", which doesn’t seem meaningful in-repo and will likely confuse future maintainers. Consider rewording the comment to describe the behavior under test (cron TZ handling) without referencing an external patch number.

Suggested change

// TestComputeNextRun_CronTimezone verifies Patch #4: cron expressions should

// respect the schedule.TZ field instead of always using UTC.

// TestComputeNextRun_CronTimezone verifies that cron expressions respect the

// schedule.TZ field instead of always using UTC.

Copilot · 2026-03-05T08:14:46Z

pkg/cron/timezone_test.go

+// TestComputeNextRun_DefaultTZ_AppliesWhenSet verifies that when TZ is empty
+// and no service default is set, the computation uses UTC.
+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")
+


This test’s doc comment says "no service default is set" and "uses UTC", but the test immediately sets a non-UTC default via SetDefaultTimezone("Asia/Shanghai"). Please update the comment to match what the test actually verifies (service default TZ applies when schedule.TZ is empty, and explicit schedule.TZ overrides it).

- Remove 'Patch sipeed#4' reference that is meaningless in-repo - Correct DefaultTZ_AppliesWhenSet doc to match actual test behavior

CLAassistant · 2026-03-05T14:58:57Z

All committers have signed the CLA.

AaronJny · 2026-03-06T01:46:46Z

recheck

yinwm · 2026-03-06T12:08:57Z

Code Review Feedback

Thanks for the contribution! This PR fixes an important issue with cron timezone handling. Here are a few suggestions:

🔴 Issues to Fix

1. Bug in Test Code (timezone_test.go:49-50)

tz := cs.defaultTZ
if tz == "" {
    tz = cs.defaultTZ  // ← BUG! Should be schedule.TZ
}

This condition checks cs.defaultTZ twice, so the if branch is never entered. Should be:

tz := schedule.TZ
if tz == "" {
    tz = cs.defaultTZ
}

2. Hardcoded Default Value Scattered

The string "UTC" is hardcoded in multiple places in service.go:

Lines 71-73: initialization
Lines 276-279: in computeNextRun

Suggestion: Define a constant at the top of the file:

const defaultTimezone = "UTC"

✅ What's Done Well

Correct fix logic: converting reference time to target timezone before computing next tick
Reasonable 3-tier fallback: schedule.TZ → config → UTC
Proper error handling: logs warning for invalid timezone and falls back to UTC

Recommend fixing the test bug and unifying the constant before merging.

yinwm · 2026-03-06T12:10:26Z

Please fix Linter bug @AaronJny

Per reviewer suggestion, replace hardcoded "UTC" with a package-level constant for maintainability.

AaronJny · 2026-03-07T02:21:13Z

Thanks for the thorough review!

Regarding #1: I think there may be a small misread in the code snippet. The actual code (service.go L271-278) is:

tz := schedule.TZ       // tier 1: job-specific TZ
if tz == "" {
    tz = cs.defaultTZ    // tier 2: server-wide default
}
if tz == "" {
    tz = defaultTimezone // tier 3: hardcoded "UTC"
}

The first line assigns schedule.TZ (not cs.defaultTZ), so the two == "" checks serve different purposes in the fallback chain:

1st check: schedule.TZ is empty → fall back to cs.defaultTZ
2nd check: cs.defaultTZ is also empty (e.g. no server-wide default configured) → fall back to "UTC"

Each tier's value can independently be empty, so each check is necessary. The TestComputeNextRun_ServiceDefaultTZ test validates this chain works correctly.

Regarding #2: Good catch! Extracted const defaultTimezone = "UTC" in d784651.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-07T02:24:21Z

pkg/cron/timezone_test.go

+			name:     "US/Eastern timezone",
+			tz:       "US/Eastern",
+			wantHour: 14, // 9:00 EST = 14:00 UTC (or 13:00 during DST)
+		},


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 · 2026-03-07T02:24:22Z

pkg/cron/timezone_test.go

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


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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-07T02:32:59Z

pkg/cron/timezone_test.go

+import (
+	"path/filepath"
+	"testing"
+	"time"


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 · 2026-03-07T02:32:59Z

pkg/cron/timezone_test.go

+			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())
+				}


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.

…nter

sipeed-bot · 2026-03-25T11:48:09Z

@AaronJny 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.

Copilot AI review requested due to automatic review settings March 4, 2026 01:53

Copilot started reviewing on behalf of AaronJny March 4, 2026 01:53 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

sipeed-bot bot added type: bug Something isn't working domain: corn domain: config labels Mar 4, 2026

yinwm reviewed Mar 4, 2026

View reviewed changes

AaronJny added 3 commits March 5, 2026 16:08

Copilot AI review requested due to automatic review settings March 5, 2026 08:10

AaronJny force-pushed the fix/cron-timezone branch from 3f4e461 to c3281a5 Compare March 5, 2026 08:10

Copilot started reviewing on behalf of AaronJny March 5, 2026 08:11 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

docs(cron): fix misleading test comments per Copilot review

038a55e

- Remove 'Patch sipeed#4' reference that is meaningless in-repo - Correct DefaultTZ_AppliesWhenSet doc to match actual test behavior

AaronJny requested a review from yinwm March 5, 2026 08:27

refactor(cron): extract defaultTimezone constant

d784651

Per reviewer suggestion, replace hardcoded "UTC" with a package-level constant for maintainability.

Copilot AI review requested due to automatic review settings March 7, 2026 02:21

Copilot started reviewing on behalf of AaronJny March 7, 2026 02:21 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 7, 2026 02:29

AaronJny force-pushed the fix/cron-timezone branch from c5913ef to 879b765 Compare March 7, 2026 02:29

Copilot started reviewing on behalf of AaronJny March 7, 2026 02:29 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

style(config): shorten CronToolsConfig struct tags to pass golines li…

6a5ee16

…nter

AaronJny force-pushed the fix/cron-timezone branch from 879b765 to 6a5ee16 Compare March 7, 2026 02:33

Alix-007 mentioned this pull request Mar 19, 2026

fix(cron): honor schedule time zones #1444

Closed

10 tasks

sipeed-bot bot closed this Mar 25, 2026

		// TestComputeNextRun_CronTimezone verifies Patch #4: cron expressions should
		// respect the schedule.TZ field instead of always using UTC.

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

Conversation

AaronJny commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description

🗣️ Type of Change

🤖 AI Code Generation

🔗 Related Issue

📚 Technical Context (Skip for Docs)

🧪 Test Environment

☑️ Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

yinwm left a comment

Choose a reason for hiding this comment

Code Review Summary

✅ What's Good

⚠️ Suggestions

1. Silent fallback on invalid timezone (Medium Priority)

2. Default timezone choice for international projects

📝 Verdict

Uh oh!

AaronJny commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronJny commented Mar 6, 2026

Uh oh!

yinwm commented Mar 6, 2026

Code Review Feedback

🔴 Issues to Fix

1. Bug in Test Code (timezone_test.go:49-50)

2. Hardcoded Default Value Scattered

✅ What's Done Well

Uh oh!

yinwm commented Mar 6, 2026

Uh oh!

AaronJny commented Mar 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 7, 2026

AaronJny commented Mar 4, 2026 •

edited

Loading

CLAassistant commented Mar 5, 2026 •

edited

Loading