Skip to content

fix(cron): respect configured timezone for naive timestamps - #12241

Closed
Julientalbot wants to merge 2 commits into
NousResearch:mainfrom
Julientalbot:fix/cron-timezone-aware-timestamps
Closed

fix(cron): respect configured timezone for naive timestamps#12241
Julientalbot wants to merge 2 commits into
NousResearch:mainfrom
Julientalbot:fix/cron-timezone-aware-timestamps

Conversation

@Julientalbot

Copy link
Copy Markdown
Contributor

Problem

When a user creates a cron job with a naive timestamp like "2026-04-18T17:30", parse_schedule() calls dt.astimezone() which interprets it in the server's system timezone. On most VPS/cloud servers this is UTC, so "17:30" fires at 17:30 UTC — not the user's local 17:30.

This is a silent bug: the cron job fires, but hours off from when the user expected. Examples:

  • Reunion (GMT+4): reminder fires 4 hours late
  • India (GMT+5:30): 5.5 hours late
  • Any non-UTC server: wrong time

The infrastructure to fix this already existshermes_time.get_timezone() reads the timezone from config.yaml or HERMES_TIMEZONE env var — but parse_schedule() never uses it.

Fix

cron/jobs.py (2 lines changed): When a naive timestamp is encountered, use _hermes_get_tz() to assign the configured timezone instead of the system timezone. Falls back to the original astimezone() behavior when no timezone is configured — zero breaking change.

hermes_cli/setup.py (~80 lines added): Add a timezone section to the setup wizard (hermes setup timezone) so users can configure their timezone during initial setup. Auto-detects the system timezone via timedatectl (Linux) or systemsetup (macOS) and validates the IANA ID.

Testing

All 55 existing cron tests pass with the patch applied.

Example

Before (VPS in UTC, user in Europe/Paris):

User: "Remind me at 17:30"
→ Cron fires at 17:30 UTC = 19:30 Paris time (2h late)

After (with timezone: "Europe/Paris" in config.yaml):

User: "Remind me at 17:30"
→ Cron fires at 17:30 Paris time ✅

If no timezone is configured → behavior unchanged (uses system timezone).

…schedule()

Problem: When a user creates a cron job with a naive timestamp like
"2026-04-18T17:30", parse_schedule() calls dt.astimezone() which
interprets it in the server's system timezone. On most VPS this is
UTC, so "17:30" fires at 17:30 UTC — not the user's local 17:30.

This is a silent bug: the cron job fires, but hours off from when
the user expected. For users in GMT+4 (Reunion), that's 4 hours
late. For GMT+5:30 (India), 5.5 hours late.

The infrastructure already exists — hermes_time.get_timezone() reads
the timezone from config.yaml or HERMES_TIMEZONE env var — but
parse_schedule() never uses it.

Fix: In parse_schedule(), when a naive timestamp is encountered,
use _hermes_get_tz() to assign the configured timezone instead of
the system timezone. Falls back to the original astimezone()
behavior when no timezone is configured.

Also adds a "timezone" section to the setup wizard (hermes setup
timezone) so users can configure this during initial setup. The
wizard auto-detects the system timezone and validates the IANA ID.

Changes:
- cron/jobs.py: import get_timezone, use it for naive timestamps
- hermes_cli/setup.py: add setup_timezone() function and section
@ParamChordiya

Copy link
Copy Markdown

Code Review: fix(cron): respect configured timezone for naive timestamps

Summary

The core fix in cron/jobs.py is correct and well-reasoned — using dt.replace(tzinfo=...) rather than localize() or astimezone() is the right approach for assigning a timezone to a naive datetime without shifting wall-clock time. The fallback preserves backward compatibility. The setup wizard addition in hermes_cli/setup.py is a nice usability improvement and follows existing patterns.

Issues

  1. No new tests for the core fix — The PR says "all 55 existing cron tests pass," but there are zero new tests covering the actual behavior change. Those tests were written before this bug was known, so by definition they don't cover it. Needs at minimum:

    • A test with HERMES_TIMEZONE set to a non-UTC timezone, verifying naive timestamps get the correct offset
    • A test with no timezone configured, confirming backward-compatible fallback
    • A test with get_timezone() returning None
  2. import subprocess inside function body — Should be at module level, or confirmed it's already imported there.

  3. zoneinfo fallback — The bare try/except Exception silently skips validation if zoneinfo isn't available. Should verify minimum Python version, or catch ImportError separately with a more helpful message.

  4. systemsetup -gettimezone may need elevated privileges on newer macOS — Consider also checking readlink /etc/localtime as a more reliable fallback:

    elif sys.platform == "darwin":
        link = Path("/etc/localtime")
        if link.is_symlink():
            target = str(link.resolve())
            if "zoneinfo/" in target:
                detected_tz = target.split("zoneinfo/", 1)[1]
  5. _hermes_get_tz() return type — Code assumes it returns a tzinfo-compatible object. If it returns a string, dt.replace(tzinfo=...) will fail at runtime. Needs a defensive check or comment confirming the contract.

  6. DST ambiguitydt.replace(tzinfo=tz) with ZoneInfo during DST transitions defaults to standard-time offset. Reasonable but worth documenting.

Verdict

Request changes — The core fix is correct, minimal, and backward-compatible. However, this PR must not merge without at least one or two unit tests covering the actual bug fix. Once tests are added, this is a clear approve.

Addresses review feedback on NousResearch#12241:
- Add 4 tests for parse_schedule naive-timestamp tz handling:
  * uses configured tz (Asia/Kolkata → UTC+5:30 offset)
  * falls back to astimezone() when no tz configured
  * guards against non-tzinfo return from _hermes_get_tz()
  * preserves explicit tz-aware input unchanged
- cron/jobs.py: isinstance(tzinfo) defensive check before dt.replace(),
  prevents opaque TypeError if get_timezone() ever returns a string
- hermes_cli/setup.py:
  * prefer /etc/localtime symlink on macOS (no elevated privileges,
    works on newer macOS where systemsetup may require sudo)
  * Linux: add /etc/localtime fallback after timedatectl + /etc/timezone
  * catch ImportError separately for zoneinfo, narrow validation catch
    to (ZoneInfoNotFoundError, ValueError)
@Julientalbot

Copy link
Copy Markdown
Contributor Author

Thanks for the review @ParamChordiya — all valid. Just pushed 44a677e addressing everything:

Tests (the main blocker) — 4 new tests in tests/cron/test_jobs.py::TestParseSchedule:

  • test_naive_iso_uses_configured_tz — Asia/Kolkata (UTC+5:30) correctly applied, wall-clock preserved, offset verified
  • test_naive_iso_fallback_when_no_tz_configured_hermes_get_tz() → None falls back to astimezone(), backward-compat
  • test_naive_iso_guards_against_non_tzinfo_return — if get_timezone() ever returns a string, we fall back safely instead of opaque TypeError
  • test_aware_iso_timestamp_preserved — explicit tz-aware input is passed through unchanged (no accidental override)

All 11 TestParseSchedule cases pass locally.

Defensive typing (#5) — added isinstance(hermes_tz, tzinfo) check in cron/jobs.py before dt.replace(), with a comment documenting the contract + the DST-ambiguity behavior (#6).

macOS detection (#4) — now prefers reading /etc/localtime symlink first (no elevated privileges, works on all recent macOS), falls back to systemsetup -gettimezone. Also added /etc/localtime as a third fallback on Linux after timedatectl and /etc/timezone.

import subprocess (#2) — kept local (it's only needed in this one setup path and setup.py already uses local imports elsewhere for subprocess), but added a comment clarifying the intent.

zoneinfo fallback (#3) — now catches ImportError separately (warns + saves unvalidated); validation except narrowed to (ZoneInfoNotFoundError, ValueError).

The unrelated test CI failures (test_concurrent_interrupt, test_command_guards, test_tts_mistral) are pre-existing regressions on main, fixed in part by #12139.

@Julientalbot

Copy link
Copy Markdown
Contributor Author

Closing — stepping back from speculative PRs on this repo to focus on public artifacts I control. The cron timezone behavior is already handled locally on my fleet via config.yaml tz override. If anyone picks this up, happy to share the test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants