From 62ef39c4b7fbf0ab7813dcfe070cdd5d557c9363 Mon Sep 17 00:00:00 2001
From: Broque Thomas <26755000+Nezreka@users.noreply.github.com>
Date: Wed, 27 May 2026 12:03:41 -0700
Subject: [PATCH] Wire automation engine through next_run_at + register
 monthly_time (PR 2/4)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PR 1 (commit 6ad85e27) shipped the ``next_run_at`` pure function as
foundation plumbing. PR 2 wires the engine through it and adds
``monthly_time`` as a real registered trigger type. After this PR
``core/automation_engine.py`` no longer has its own datetime
arithmetic for daily / weekly schedules — every next-run computation
flows through one function with one set of defensive fallbacks.

Net user-visible change: zero (no UI surface for monthly_time yet —
that's PR 3). New ``monthly_time`` trigger is reachable only via
direct API for now.

**Engine refactor:**

- ``_finish_run`` — collapsed three inline branches (daily_time
  arithmetic, weekly_time arithmetic, fallback schedule arithmetic)
  into a single ``next_run_at(...)`` call with ``_dt_to_db_str``
  normalising the aware-UTC result to the engine's naive-UTC string
  convention. Retry-delay short-circuit preserved. Exception
  swallowing preserved (logged at debug, writes None next_run).

- ``_setup_daily_time_trigger`` + ``_setup_weekly_time_trigger`` +
  new ``_setup_monthly_time_trigger`` — three near-identical methods
  collapsed into one ``_setup_timed_trigger`` skeleton. Each public
  method is now a one-line dispatch passing trigger_type to the
  shared helper with a human-readable label for the debug log.

- Existing ``_next_weekly_occurrence`` deleted — its logic now lives
  in ``core/automation/schedule.py:_next_weekly`` (lifted in PR 1).

- New ``_dt_to_db_str(dt)`` module-level helper normalises aware-UTC
  → naive-UTC string. Centralised so a tz mistake here surfaces in
  one place. Aware non-UTC datetimes converted to UTC first
  (defensive against a future bug that passes the wrong tz).

- New ``_resolve_system_default_tz()`` reads the server's local IANA
  tz via ``tzlocal``. Cached at module import (the host's tz doesn't
  change while the process runs). Falls back to UTC when ``tzlocal``
  is missing — defensive for minimal Docker images.

- New ``self._default_tz`` engine attribute reads from
  ``automation.default_timezone`` config first, falls back to the
  system-detected IANA name. Override path lets users on weird
  setups pin a specific tz without touching env vars.

**Convergence fix (intentional behaviour change):**

Old ``_setup_daily_time_trigger`` / ``_setup_weekly_time_trigger``
didn't check the DB for an existing future ``next_run`` — they'd
recompute from scratch on every engine startup, overwriting manual
edits or pending retries. The interval path (``_setup_schedule_trigger``)
already had this check. The new shared ``_setup_timed_trigger``
brings daily / weekly in line: existing-future next_run wins over
freshly-computed delay. Treat this as a correctness fix, not a
breaking change — the old behaviour was an inconsistency, not a
deliberate choice.

**Backward-compat:**

- Existing ``schedule`` / ``daily_time`` / ``weekly_time`` rows
  continue to work unchanged. The ``_trigger_handlers`` registry
  keeps every historic key.

- Existing rows without an explicit ``tz`` field use
  ``self._default_tz`` (server-local IANA via ``tzlocal``) —
  preserves "every Monday 09:00 server-local" behaviour on
  non-UTC servers. Pre-fix the engine used naive
  ``datetime.now()`` which is also server-local; net effect is
  identical wall-clock time, just routed through a tz-aware
  pipeline that handles DST correctly (the May 2026 "next in 8h"
  bug fix class).

- Engine boots even when ``tzlocal`` is missing — the resolver
  falls back to UTC silently. Existing tests would catch a hard
  dependency on tzlocal here.

**``tzlocal>=5.0`` added to requirements.txt** alongside
``tzdata>=2024.1`` from PR 1. Both libraries are small and stable;
``tzlocal`` returns a clean IANA name across Windows / Linux /
Docker, sidestepping the platform-specific tz detection mess.

**Tests:** 20 new in ``tests/automation/test_engine_schedule_integration.py``:
- ``_dt_to_db_str`` x3 (aware UTC, aware non-UTC converted to UTC,
  naive assumed UTC)
- ``_resolve_system_default_tz`` x2 (returns IANA string, falls back
  to UTC without tzlocal)
- ``_finish_run`` dispatch through next_run_at for each trigger type
  (schedule, daily_time, weekly_time, monthly_time)
- Retry-delay short-circuits next_run_at
- next_run_at returns None → DB next_run cleared
- next_run_at raises → engine swallows + writes None
- Event triggers skipped (no scheduled next-run)
- ``self._default_tz`` passed through to next_run_at
- monthly_time registered in _trigger_handlers
- All historic trigger types kept registered
- ``_setup_monthly_time_trigger`` arms timer + writes DB
- ``_setup_timed_trigger`` honours existing future DB next_run
- Skip-with-log when next_run_at returns None
- End-to-end no-mock smoke for monthly_time

260 automation suite tests pass; the 240 from PR 1's branch plus 20
new integration tests. Ruff clean.

No WHATS_NEW entry — UI doesn't expose monthly_time yet (PR 3),
and the backward-compat path preserves existing daily/weekly
schedule timing.
---
 core/automation_engine.py                     | 201 ++++++---
 requirements.txt                              |   9 +
 .../test_engine_schedule_integration.py       | 405 ++++++++++++++++++
 3 files changed, 545 insertions(+), 70 deletions(-)
 create mode 100644 tests/automation/test_engine_schedule_integration.py

diff --git a/core/automation_engine.py b/core/automation_engine.py
index c04c1760..5264c9c5 100644
--- a/core/automation_engine.py
+++ b/core/automation_engine.py
@@ -18,10 +18,14 @@ import time
 import threading
 import requests
 from datetime import datetime, timedelta, timezone
+from typing import Optional
 from utils.logging_config import get_logger
 
+from core.automation.schedule import next_run_at
+
 logger = get_logger("automation_engine")
 
+
 def _utcnow():
     """Return current UTC time as timezone-aware datetime."""
     return datetime.now(timezone.utc)
@@ -34,6 +38,42 @@ def _utc_after(seconds):
     """Return UTC time N seconds from now as naive string for DB storage."""
     return (datetime.now(timezone.utc) + timedelta(seconds=seconds)).strftime('%Y-%m-%d %H:%M:%S')
 
+
+def _dt_to_db_str(dt: datetime) -> str:
+    """Convert an aware-UTC datetime to the naive-UTC string the DB
+    ``next_run`` column stores. Centralised so a tz mistake here
+    surfaces in one place, not scattered through every caller of
+    ``next_run_at``."""
+    if dt.tzinfo is None:
+        return dt.strftime('%Y-%m-%d %H:%M:%S')
+    return dt.astimezone(timezone.utc).strftime('%Y-%m-%d %H:%M:%S')
+
+
+def _resolve_system_default_tz() -> str:
+    """Return the IANA tz name the engine uses when a schedule's
+    trigger config doesn't carry an explicit ``tz`` field.
+
+    Existing daily / weekly rows pre-date the ``tz`` field — the
+    historic engine computed delays from naive ``datetime.now()``,
+    which is implicitly the server's local timezone. Falling back to
+    that same tz here preserves "every Monday at 09:00" running at
+    09:00 server local for rows that already exist in the DB.
+    Without ``tzlocal`` installed (or a system without a discoverable
+    tz), falls back to UTC."""
+    try:
+        import tzlocal
+        return tzlocal.get_localzone_name() or 'UTC'
+    except Exception:
+        return 'UTC'
+
+
+# Server-local tz cached at import time. Re-reading per-call is
+# pointless: the host's timezone doesn't change while the process is
+# running. Tests that need a different default tz inject it through
+# the engine's ``_default_tz`` attribute or via the
+# ``automation.default_timezone`` config key.
+_SYSTEM_DEFAULT_TZ = _resolve_system_default_tz()
+
 SYSTEM_AUTOMATIONS = [
     {
         'name': 'Auto-Process Wishlist',
@@ -134,11 +174,25 @@ class AutomationEngine:
         self._max_chain_depth = 5
         self._signal_cooldown_seconds = 10
 
+        # Default tz used when a schedule's ``trigger_config`` doesn't
+        # carry an explicit ``tz`` field — preserves historic behaviour
+        # for daily / weekly rows created before the field existed
+        # (engine used naive ``datetime.now()`` = server local). Reads
+        # from the ``automation.default_timezone`` config key first to
+        # let users override without touching env vars; falls back to
+        # the system-detected local tz.
+        try:
+            from config.settings import config_manager
+            self._default_tz = (config_manager.get('automation.default_timezone', '') or _SYSTEM_DEFAULT_TZ)
+        except Exception:
+            self._default_tz = _SYSTEM_DEFAULT_TZ
+
         # Trigger registry: type → setup function (schedule only — events use emit())
         self._trigger_handlers = {
             'schedule': self._setup_schedule_trigger,
             'daily_time': self._setup_daily_time_trigger,
             'weekly_time': self._setup_weekly_time_trigger,
+            'monthly_time': self._setup_monthly_time_trigger,
         }
 
     # --- Action Handler Registration ---
@@ -674,23 +728,21 @@ class AutomationEngine:
                 trigger_config = json.loads(auto.get('trigger_config') or '{}')
                 if retry_delay_seconds:
                     next_run_str = _utc_after(retry_delay_seconds)
-                elif trigger_type == 'daily_time':
-                    # Next run is tomorrow at the configured time (compute delay from local time, store as UTC)
-                    time_str = trigger_config.get('time', '00:00')
-                    hour, minute = map(int, time_str.split(':'))
-                    now_local = datetime.now()
-                    target = now_local.replace(hour=hour, minute=minute, second=0, microsecond=0) + timedelta(days=1)
-                    next_run_str = _utc_after((target - now_local).total_seconds())
-                elif trigger_type == 'weekly_time':
-                    time_str = trigger_config.get('time', '00:00')
-                    hour, minute = map(int, time_str.split(':'))
-                    now_local = datetime.now()
-                    target = self._next_weekly_occurrence(hour, minute, trigger_config.get('days', []))
-                    next_run_str = _utc_after((target - now_local).total_seconds())
                 else:
-                    delay = self._calc_delay_seconds(trigger_config)
-                    if delay:
-                        next_run_str = _utc_after(delay)
+                    # Single integration point with ``next_run_at``. The
+                    # helper handles every trigger type the engine
+                    # supports (interval / daily / weekly / monthly) and
+                    # returns aware-UTC; ``_dt_to_db_str`` normalises to
+                    # the naive-UTC string the DB column stores. Tests
+                    # injecting a different ``now_utc`` patch this same
+                    # path — no scattered ``datetime.now()`` calls left.
+                    next_run_dt = next_run_at(
+                        trigger_type, trigger_config,
+                        now_utc=_utcnow(),
+                        default_tz=self._default_tz,
+                    )
+                    if next_run_dt is not None:
+                        next_run_str = _dt_to_db_str(next_run_dt)
             except Exception as e:
                 logger.debug("next run calc failed: %s", e)
 
@@ -764,45 +816,72 @@ class AutomationEngine:
         logger.debug(f"Scheduled automation {automation_id} in {delay:.0f}s")
 
     def _setup_daily_time_trigger(self, automation_id, config):
-        """Config: {"time": "03:00"}  — runs daily at the specified local time."""
-        time_str = config.get('time', '00:00')
-        try:
-            hour, minute = map(int, time_str.split(':'))
-        except (ValueError, AttributeError):
-            hour, minute = 0, 0
-
-        now_local = datetime.now()
-        target = now_local.replace(hour=hour, minute=minute, second=0, microsecond=0)
-        if target <= now_local:
-            target += timedelta(days=1)
-
-        delay = (target - now_local).total_seconds()
-
-        next_run_str = _utc_after(delay)
-        self.db.update_automation(automation_id, next_run=next_run_str)
-
-        timer = threading.Timer(delay, self.run_automation, args=(automation_id,))
-        timer.daemon = True
-        timer.start()
-
-        with self._lock:
-            self._timers[automation_id] = timer
-
-        logger.debug(f"Daily automation {automation_id} scheduled for {time_str} (in {delay:.0f}s)")
+        """Config: ``{"time": "03:00", "tz": "<IANA>"}`` — runs daily
+        at the specified local time. Tz defaults to ``self._default_tz``
+        when absent."""
+        self._setup_timed_trigger(automation_id, 'daily_time', config,
+                                  label=f"Daily at {config.get('time', '00:00')}")
 
     def _setup_weekly_time_trigger(self, automation_id, config):
-        """Config: {"time": "03:00", "days": ["mon", "wed", "fri"]}"""
-        time_str = config.get('time', '00:00')
-        try:
-            hour, minute = map(int, time_str.split(':'))
-        except (ValueError, AttributeError):
-            hour, minute = 0, 0
+        """Config: ``{"time": "03:00", "days": ["mon","wed","fri"], "tz": "<IANA>"}``."""
+        day_names = ', '.join(config.get('days') or []) or 'every day'
+        self._setup_timed_trigger(automation_id, 'weekly_time', config,
+                                  label=f"Weekly {config.get('time', '00:00')} on {day_names}")
+
+    def _setup_monthly_time_trigger(self, automation_id, config):
+        """Config: ``{"time": "09:00", "day_of_month": 15, "tz": "<IANA>"}``.
+
+        Day clamped to [1, 31]; months too short for the target day
+        clamp to the last valid day (Feb 31 → Feb 28 / Feb 29 leap
+        year) per standard cron convention — see
+        ``core.automation.schedule._next_monthly`` for the rule."""
+        day = config.get('day_of_month', 1)
+        self._setup_timed_trigger(automation_id, 'monthly_time', config,
+                                  label=f"Monthly {config.get('time', '00:00')} on day {day}")
+
+    def _setup_timed_trigger(self, automation_id, trigger_type, config, *, label):
+        """Shared setup for daily / weekly / monthly time triggers.
+
+        All three flow through the same skeleton: compute next-run
+        via ``next_run_at``, persist to DB, arm a ``threading.Timer``
+        that fires the automation when the delay elapses. Lifting
+        these out of three near-identical methods means there's one
+        place to fix when (e.g.) timer rearm semantics need a tweak.
+
+        Honours an existing future ``next_run`` row in the DB —
+        prevents losing a hand-edited next_run when the engine
+        reschedules at startup. Same guard as the interval path."""
+        target_dt = next_run_at(
+            trigger_type, config or {},
+            now_utc=_utcnow(),
+            default_tz=self._default_tz,
+        )
+        if target_dt is None:
+            logger.warning(
+                f"Skip scheduling automation {automation_id}: next_run_at returned "
+                f"None for {trigger_type!r}",
+            )
+            return
 
-        target = self._next_weekly_occurrence(hour, minute, config.get('days', []))
-        delay = (target - datetime.now()).total_seconds()
+        delay = max(0.0, (target_dt - _utcnow()).total_seconds())
 
-        next_run_str = _utc_after(delay)
-        self.db.update_automation(automation_id, next_run=next_run_str)
+        # If the DB already carries a future next_run, prefer it —
+        # matches the interval-path behaviour and lets manual edits
+        # or pending retries survive a process restart.
+        auto = self.db.get_automation(automation_id)
+        if auto and auto.get('next_run'):
+            try:
+                existing = datetime.strptime(
+                    auto['next_run'], '%Y-%m-%d %H:%M:%S',
+                ).replace(tzinfo=timezone.utc)
+                remaining = (existing - _utcnow()).total_seconds()
+                if remaining > 0:
+                    delay = remaining
+                    target_dt = existing
+            except (ValueError, TypeError):
+                pass
+
+        self.db.update_automation(automation_id, next_run=_dt_to_db_str(target_dt))
 
         timer = threading.Timer(delay, self.run_automation, args=(automation_id,))
         timer.daemon = True
@@ -811,25 +890,7 @@ class AutomationEngine:
         with self._lock:
             self._timers[automation_id] = timer
 
-        day_names = ', '.join(config.get('days', [])) or 'every day'
-        logger.debug(f"Weekly automation {automation_id} scheduled for {time_str} on {day_names} (in {delay:.0f}s)")
-
-    def _next_weekly_occurrence(self, hour, minute, days):
-        """Find the next datetime matching one of the given weekday abbreviations."""
-        day_map = {'mon': 0, 'tue': 1, 'wed': 2, 'thu': 3, 'fri': 4, 'sat': 5, 'sun': 6}
-        allowed = {day_map[d] for d in days if d in day_map}
-        if not allowed:
-            allowed = set(range(7))  # no days selected = every day
-
-        now = datetime.now()
-        for offset in range(8):  # check today + next 7 days
-            candidate = now + timedelta(days=offset)
-            if candidate.weekday() in allowed:
-                target = candidate.replace(hour=hour, minute=minute, second=0, microsecond=0)
-                if target > now:
-                    return target
-        # Fallback: tomorrow (shouldn't happen with 8-day scan)
-        return now.replace(hour=hour, minute=minute, second=0, microsecond=0) + timedelta(days=1)
+        logger.debug(f"{label} automation {automation_id} scheduled (in {delay:.0f}s)")
 
     # --- Then Actions (notifications + signals) ---
 
diff --git a/requirements.txt b/requirements.txt
index b1fda9f0..bc1ade54 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -37,6 +37,15 @@ psutil==7.2.2
 # the app's tz knowledge to the build date.
 tzdata>=2024.1
 
+# Cross-platform IANA timezone detection — returns the server's local
+# tz as a string ('America/Los_Angeles', not 'PDT'). Consumed by the
+# automation engine to preserve historic behaviour for daily / weekly
+# trigger rows that don't carry an explicit ``tz`` field: the old
+# engine computed delays from naive ``datetime.now()``, which is
+# implicitly the server's local tz, so falling back to the same tz
+# keeps existing schedules running at the same wall-clock time.
+tzlocal>=5.0
+
 # YouTube support -- unpinned; yt-dlp must track upstream releases to stay functional
 yt-dlp>=2026.3.17
 
diff --git a/tests/automation/test_engine_schedule_integration.py b/tests/automation/test_engine_schedule_integration.py
new file mode 100644
index 00000000..11fde011
--- /dev/null
+++ b/tests/automation/test_engine_schedule_integration.py
@@ -0,0 +1,405 @@
+"""Engine-level integration tests for the next_run_at refactor (PR 2).
+
+PR 1 lifted next-run computation into ``core/automation/schedule.py``
+as a pure function. PR 2 wires the engine through it — three setup
+methods (daily / weekly / monthly) collapse to one ``_setup_timed_trigger``
+helper, ``_finish_run`` drops its inline daily / weekly arithmetic,
+and ``monthly_time`` becomes a real registered trigger type.
+
+These tests pin the integration surface:
+- ``_finish_run`` dispatches through ``next_run_at`` for every trigger
+  type with the right args (trigger_type, trigger_config, now_utc,
+  default_tz) and serialises the result into the DB ``next_run`` column.
+- Retry-delay short-circuit bypasses ``next_run_at`` (immediate
+  reschedule on transient failure, not on the regular cadence).
+- Error path swallows + writes None next_run instead of crashing.
+- Backward-compat: existing daily / weekly rows without an explicit
+  ``tz`` field use the engine's ``_default_tz`` (server-local IANA),
+  preserving "every Monday 09:00 server-local" behaviour.
+- New ``monthly_time`` trigger registers in ``_trigger_handlers`` and
+  arms a timer correctly.
+- ``_setup_timed_trigger`` honours an existing future ``next_run`` in
+  the DB (lets manual edits / restart-resume survive).
+- ``_dt_to_db_str`` correctly normalises aware + naive datetimes to
+  the engine's naive-UTC string convention.
+"""
+
+from __future__ import annotations
+
+import json
+from datetime import datetime, timedelta, timezone
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from core.automation_engine import (
+    AutomationEngine,
+    _dt_to_db_str,
+    _resolve_system_default_tz,
+)
+
+
+def _utc(year, month, day, hour=0, minute=0, second=0):
+    """Aware UTC datetime for test clarity — matches the convention
+    used in tests/automation/test_schedule.py."""
+    return datetime(year, month, day, hour, minute, second, tzinfo=timezone.utc)
+
+
+# ---------------------------------------------------------------------------
+# _dt_to_db_str — engine-side serialiser for ``next_run_at`` results.
+# ---------------------------------------------------------------------------
+
+
+def test_dt_to_db_str_normalises_aware_utc():
+    """Aware-UTC datetime → naive-UTC string the DB column expects.
+    Format matches the engine's existing ``_utc_after``."""
+    dt = _utc(2026, 5, 27, 14, 30, 0)
+    assert _dt_to_db_str(dt) == '2026-05-27 14:30:00'
+
+
+def test_dt_to_db_str_converts_aware_non_utc_to_utc_first():
+    """An aware datetime in a non-UTC tz must be converted to UTC
+    before stringifying — otherwise the DB column would silently
+    hold a tz-shifted instant. This is the bug class the PR 1
+    tests already cover at the next_run_at layer; pin it here so a
+    future refactor that drops the ``.astimezone(UTC)`` step fails
+    fast."""
+    from zoneinfo import ZoneInfo
+    la = ZoneInfo('America/Los_Angeles')
+    dt = datetime(2026, 5, 27, 9, 0, 0, tzinfo=la)  # 09:00 PDT
+    # 09:00 PDT (UTC-7) → 16:00 UTC.
+    assert _dt_to_db_str(dt) == '2026-05-27 16:00:00'
+
+
+def test_dt_to_db_str_assumes_naive_is_utc():
+    """Defensive — naive inputs are assumed UTC (matches the engine's
+    convention when parsing the DB column back out)."""
+    dt = datetime(2026, 5, 27, 14, 30, 0)  # naive
+    assert _dt_to_db_str(dt) == '2026-05-27 14:30:00'
+
+
+# ---------------------------------------------------------------------------
+# _resolve_system_default_tz — engine's tz fallback chain.
+# ---------------------------------------------------------------------------
+
+
+def test_resolve_system_default_tz_returns_iana_string():
+    """The engine caches this at import time; the result must be a
+    string (not a ZoneInfo object) so it can flow into next_run_at's
+    ``default_tz`` param."""
+    result = _resolve_system_default_tz()
+    assert isinstance(result, str)
+    assert len(result) > 0
+
+
+def test_resolve_system_default_tz_falls_back_to_utc_when_tzlocal_missing():
+    """tzlocal is in requirements but the engine should still boot
+    without it — minimal Docker images / dev environments where
+    tzlocal didn't install. Defensive fallback to UTC instead of
+    crashing the engine."""
+    with patch.dict('sys.modules', {'tzlocal': None}):
+        # Force ImportError on the in-function import.
+        import importlib
+        import core.automation_engine as engine_mod
+        importlib.reload(engine_mod)
+        result = engine_mod._resolve_system_default_tz()
+        assert result == 'UTC'
+        # Reload again to restore normal state for subsequent tests.
+        importlib.reload(engine_mod)
+
+
+# ---------------------------------------------------------------------------
+# Engine fixture — minimal AutomationEngine with mocked DB.
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def engine_with_db():
+    """AutomationEngine wired to a mock DB. Used across the
+    integration tests below — each test sets ``trigger_type`` and
+    ``trigger_config`` on the mock's ``get_automation`` return value."""
+    db_mock = MagicMock()
+    db_mock.update_automation_run = MagicMock(return_value=True)
+    db_mock.update_automation = MagicMock(return_value=True)
+    db_mock.get_automation.return_value = None  # tests override
+    engine = AutomationEngine(db_mock)
+    engine._running = True
+    return engine, db_mock
+
+
+# ---------------------------------------------------------------------------
+# _finish_run — single integration point with next_run_at.
+# ---------------------------------------------------------------------------
+
+
+def test_finish_run_dispatches_interval_trigger_through_next_run_at(engine_with_db):
+    """Interval trigger flows through the same next_run_at call as
+    daily/weekly/monthly — no special-case branch left in the engine
+    for the legacy ``schedule`` type."""
+    engine, db_mock = engine_with_db
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'schedule',
+        'trigger_config': json.dumps({'interval': 6, 'unit': 'hours'}),
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        mock_nra.return_value = _utc(2026, 5, 27, 18, 0)
+        engine._finish_run(auto, 1, {'status': 'completed'}, error=None)
+    assert mock_nra.called
+    call = mock_nra.call_args
+    assert call.args[0] == 'schedule'
+    assert call.args[1] == {'interval': 6, 'unit': 'hours'}
+    assert call.kwargs['default_tz'] == engine._default_tz
+
+
+def test_finish_run_dispatches_daily_time_through_next_run_at(engine_with_db):
+    """Daily trigger no longer has its own inline arithmetic — the
+    refactor must route through next_run_at with the unmodified
+    trigger_config so tz / time fields flow through cleanly."""
+    engine, db_mock = engine_with_db
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'daily_time',
+        'trigger_config': json.dumps({'time': '09:00', 'tz': 'America/Los_Angeles'}),
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        mock_nra.return_value = _utc(2026, 5, 27, 16, 0)
+        engine._finish_run(auto, 1, {}, error=None)
+    assert mock_nra.call_args.args[0] == 'daily_time'
+    assert mock_nra.call_args.args[1] == {'time': '09:00', 'tz': 'America/Los_Angeles'}
+
+
+def test_finish_run_dispatches_weekly_time_through_next_run_at(engine_with_db):
+    """Weekly trigger same as daily — single integration point."""
+    engine, db_mock = engine_with_db
+    cfg = {'time': '09:00', 'days': ['mon', 'wed', 'fri'], 'tz': 'America/Los_Angeles'}
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'weekly_time',
+        'trigger_config': json.dumps(cfg),
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        mock_nra.return_value = _utc(2026, 5, 27, 16, 0)
+        engine._finish_run(auto, 1, {}, error=None)
+    assert mock_nra.call_args.args[0] == 'weekly_time'
+    assert mock_nra.call_args.args[1] == cfg
+
+
+def test_finish_run_dispatches_monthly_time_through_next_run_at(engine_with_db):
+    """New monthly_time trigger — added to _trigger_handlers in PR 2.
+    Without this entry, the if-trigger_type-in-handlers gate above
+    skips computation entirely and the DB next_run stays stale."""
+    engine, db_mock = engine_with_db
+    cfg = {'time': '09:00', 'day_of_month': 15, 'tz': 'America/Los_Angeles'}
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'monthly_time',
+        'trigger_config': json.dumps(cfg),
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        mock_nra.return_value = _utc(2026, 6, 15, 16, 0)
+        engine._finish_run(auto, 1, {}, error=None)
+    assert mock_nra.call_args.args[0] == 'monthly_time'
+
+
+def test_finish_run_retry_delay_short_circuits_next_run_at(engine_with_db):
+    """When a transient failure asks for a retry-delay reschedule
+    (e.g. action handler returns ``status='retry'``), the next_run
+    is just now+delay — NOT the regular schedule cadence. The
+    refactor must preserve this short-circuit path."""
+    engine, db_mock = engine_with_db
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'daily_time',
+        'trigger_config': json.dumps({'time': '09:00'}),
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        engine._finish_run(auto, 1, {}, error='boom', retry_delay_seconds=120)
+    # next_run_at NOT called — we used the retry delay instead.
+    mock_nra.assert_not_called()
+    # DB write happened (with a next_run computed from now + 120s).
+    assert db_mock.update_automation_run.called
+    written = db_mock.update_automation_run.call_args.kwargs.get('next_run')
+    assert written is not None
+
+
+def test_finish_run_writes_none_when_next_run_at_returns_none(engine_with_db):
+    """Defensive — next_run_at can return None for unknown trigger
+    types or completely broken configs. The engine must write
+    None to the DB rather than skip the update (which would leave
+    a stale next_run sitting in the row forever)."""
+    engine, db_mock = engine_with_db
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'daily_time',
+        'trigger_config': json.dumps({'time': '09:00'}),
+    }
+    with patch('core.automation_engine.next_run_at', return_value=None):
+        engine._finish_run(auto, 1, {}, error=None)
+    assert db_mock.update_automation_run.called
+    assert db_mock.update_automation_run.call_args.kwargs.get('next_run') is None
+
+
+def test_finish_run_swallows_next_run_at_exception(engine_with_db):
+    """next_run_at is pure so it shouldn't raise — but if it does
+    (programmer error in the helper, weird tz lookup), the engine
+    must not crash the finish-run cycle. Existing behaviour
+    swallows + logs at debug; the refactor preserves that."""
+    engine, db_mock = engine_with_db
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'daily_time',
+        'trigger_config': json.dumps({'time': '09:00'}),
+    }
+    with patch('core.automation_engine.next_run_at', side_effect=RuntimeError('boom')):
+        engine._finish_run(auto, 1, {}, error=None)
+    # DB write still happens, just with None next_run.
+    assert db_mock.update_automation_run.called
+
+
+def test_finish_run_skips_next_run_for_event_triggers(engine_with_db):
+    """Event-based triggers (not in _trigger_handlers) have no
+    scheduled next-run — the existing gate must still skip them
+    after the refactor."""
+    engine, db_mock = engine_with_db
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'event',
+        'trigger_config': json.dumps({}),
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        engine._finish_run(auto, 1, {}, error=None)
+    mock_nra.assert_not_called()
+    # update_automation_run still fires but with next_run=None.
+    assert db_mock.update_automation_run.call_args.kwargs.get('next_run') is None
+
+
+def test_finish_run_passes_engine_default_tz(engine_with_db):
+    """Backward-compat: existing daily/weekly rows without ``tz`` in
+    their config must use the engine's ``_default_tz`` (server-local
+    IANA via tzlocal). Pre-fix, the engine implicitly used naive
+    ``datetime.now()`` = server local; post-fix the explicit
+    default_tz preserves that behaviour."""
+    engine, db_mock = engine_with_db
+    engine._default_tz = 'America/Los_Angeles'  # simulate server-local
+    auto = {
+        'id': 1, 'enabled': True,
+        'trigger_type': 'daily_time',
+        'trigger_config': json.dumps({'time': '09:00'}),  # NO tz field
+    }
+    with patch('core.automation_engine.next_run_at') as mock_nra:
+        mock_nra.return_value = _utc(2026, 5, 27, 16, 0)
+        engine._finish_run(auto, 1, {}, error=None)
+    assert mock_nra.call_args.kwargs['default_tz'] == 'America/Los_Angeles'
+
+
+# ---------------------------------------------------------------------------
+# Trigger handler registration.
+# ---------------------------------------------------------------------------
+
+
+def test_engine_registers_monthly_time_trigger(engine_with_db):
+    """``monthly_time`` joins schedule / daily_time / weekly_time in
+    the _trigger_handlers registry — without this, calling
+    ``schedule_automation`` on a monthly row falls through the
+    ``trigger_type in self._trigger_handlers`` gate and the
+    automation never gets armed."""
+    engine, _ = engine_with_db
+    assert 'monthly_time' in engine._trigger_handlers
+    assert callable(engine._trigger_handlers['monthly_time'])
+
+
+def test_engine_keeps_existing_trigger_registrations(engine_with_db):
+    """Backward-compat: the refactor must not drop the historic
+    trigger types. schedule / daily_time / weekly_time stay
+    registered alongside the new monthly_time."""
+    engine, _ = engine_with_db
+    assert 'schedule' in engine._trigger_handlers
+    assert 'daily_time' in engine._trigger_handlers
+    assert 'weekly_time' in engine._trigger_handlers
+
+
+# ---------------------------------------------------------------------------
+# _setup_timed_trigger — shared skeleton for daily / weekly / monthly.
+# ---------------------------------------------------------------------------
+
+
+def test_setup_monthly_time_trigger_writes_next_run_and_arms_timer(engine_with_db):
+    """Sanity check that the new monthly handler actually wires up
+    a timer (it's the new-shaped trigger so a "no timer armed"
+    regression would otherwise be silent — the automation just
+    never fires)."""
+    engine, db_mock = engine_with_db
+    db_mock.get_automation.return_value = {'id': 1, 'next_run': None}
+    with patch('core.automation_engine.threading.Timer') as mock_timer_cls:
+        mock_timer = MagicMock()
+        mock_timer_cls.return_value = mock_timer
+        engine._setup_monthly_time_trigger(
+            1, {'time': '09:00', 'day_of_month': 15, 'tz': 'UTC'},
+        )
+    # Timer armed.
+    assert mock_timer.start.called
+    # next_run written to DB.
+    assert db_mock.update_automation.called
+    written = db_mock.update_automation.call_args.kwargs.get('next_run')
+    assert written is not None and isinstance(written, str)
+
+
+def test_setup_timed_trigger_honours_future_db_next_run(engine_with_db):
+    """If the DB row already has a future ``next_run`` (e.g. a
+    manual edit, or a process restart picking up where it left
+    off), the setup must keep that instant — not recompute from
+    scratch. Matches the existing interval-path behaviour and
+    prevents losing pending retries."""
+    engine, db_mock = engine_with_db
+    # Far-future next_run in the DB.
+    future = (datetime.now(timezone.utc) + timedelta(hours=24)).strftime('%Y-%m-%d %H:%M:%S')
+    db_mock.get_automation.return_value = {'id': 1, 'next_run': future}
+    with patch('core.automation_engine.threading.Timer') as mock_timer_cls:
+        mock_timer_cls.return_value = MagicMock()
+        engine._setup_daily_time_trigger(1, {'time': '09:00', 'tz': 'UTC'})
+    # Engine writes the EXISTING next_run back (the if-future-in-DB
+    # branch overrides the freshly-computed delay).
+    written = db_mock.update_automation.call_args.kwargs.get('next_run')
+    assert written == future
+
+
+def test_setup_timed_trigger_skips_when_next_run_at_returns_none(engine_with_db):
+    """If next_run_at can't compute a valid next-run (e.g. broken
+    config that defeats every defensive fallback in the helper),
+    the setup must NOT arm a timer with bogus delay. Skip-with-log
+    is safer than scheduling-for-the-past or scheduling-immediately."""
+    engine, db_mock = engine_with_db
+    db_mock.get_automation.return_value = {'id': 1, 'next_run': None}
+    with patch('core.automation_engine.next_run_at', return_value=None), \
+         patch('core.automation_engine.threading.Timer') as mock_timer_cls:
+        engine._setup_monthly_time_trigger(1, {})
+    # No timer armed.
+    mock_timer_cls.assert_not_called()
+
+
+# ---------------------------------------------------------------------------
+# End-to-end: real next_run_at + engine wiring (no mocks).
+# ---------------------------------------------------------------------------
+
+
+def test_end_to_end_monthly_schedule_produces_valid_db_string(engine_with_db):
+    """No-mock smoke: monthly_time config flows from engine through
+    the real next_run_at into a valid DB string. Catches any
+    serialisation drift between PR 1 (helper returns aware UTC) and
+    PR 2 (engine writes naive UTC string)."""
+    engine, db_mock = engine_with_db
+    engine._default_tz = 'UTC'
+    db_mock.get_automation.return_value = {'id': 1, 'next_run': None}
+    with patch('core.automation_engine.threading.Timer') as mock_timer_cls:
+        mock_timer_cls.return_value = MagicMock()
+        engine._setup_monthly_time_trigger(
+            1, {'time': '09:00', 'day_of_month': 15},
+        )
+    written = db_mock.update_automation.call_args.kwargs['next_run']
+    # Format matches the engine's existing _utcnow_str / _utc_after.
+    parsed = datetime.strptime(written, '%Y-%m-%d %H:%M:%S')
+    # Day-of-month is 15 in the user's tz (UTC here).
+    assert parsed.day == 15
+    assert parsed.hour == 9
+    assert parsed.minute == 0