From 345bd200e17ddab51ab825e91785aab71c195c64 Mon Sep 17 00:00:00 2001 From: Joshua Date: Mon, 21 Sep 2026 15:19:09 -0400 Subject: [PATCH 1/4] Replace HA config reads with an explicit settings object (task 4dd8f870) Add core/settings.py: a frozen, HA-free NestQuestSettings dataclass carrying horizon_days, day_rollover_time, notify_target, the three notification times, and the four enable toggles. from_options() reads the CONF_* keys with the matching defaults and validates (horizon int >= 1, strict HH:MM times, real booleans); horizon_window(today) returns the rolling [today, today+horizon_days] window. The integration builds the settings object once from entry.options and threads it into the horizon materialization and daily-rollover listener; core never reads HA config itself. A settings.py shim re-exports core.settings so the integration keeps its shim-only import convention. Tests: an AST scan asserts no core module imports homeassistant or accesses hass.config / entry.options / entry.data; a from_options test covers defaults-when-absent and rejection of invalid horizon/rollover; a materialization test drives a non-default horizon (3 days, 03:30 rollover) and asserts the resulting instance window matches. --- custom_components/nestquest/__init__.py | 92 ++-- custom_components/nestquest/core/settings.py | 302 +++++++++++ custom_components/nestquest/settings.py | 13 + tests/test_core_settings.py | 504 +++++++++++++++++++ 4 files changed, 874 insertions(+), 37 deletions(-) create mode 100644 custom_components/nestquest/core/settings.py create mode 100644 custom_components/nestquest/settings.py create mode 100644 tests/test_core_settings.py diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 0b1ef1e..abea786 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -18,16 +18,14 @@ from homeassistant.helpers.event import async_track_time_change from .const import ( CONF_ADMIN_USER_IDS, - CONF_DAY_ROLLOVER_TIME, - CONF_HORIZON_DAYS, CONF_UPDATE_INTERVAL, - DEFAULT_DAY_ROLLOVER_TIME, DEFAULT_HORIZON_DAYS, DEFAULT_UPDATE_INTERVAL, DOMAIN, LOGGER, PLATFORMS, ) +from .settings import NestQuestSettings from .frontend import async_register_frontend from .coordinator import NestQuestCoordinator from .db import NestQuestDatabase, make_database @@ -241,6 +239,7 @@ class NestQuestRuntimeData: entry_id: str options: dict[str, Any] + settings: NestQuestSettings database: NestQuestDatabase remove_update_listener: Callable[[], Any] remove_time_change_listener: Callable[[], Any] @@ -272,35 +271,40 @@ async def _async_owner_user_ids(hass: HomeAssistant) -> list[str]: def _day_rollover_hour_minute(options: dict[str, Any]) -> tuple[int, int]: """Return the (hour, minute) the daily rollover fires at from ``options``. - The options flow validates ``day_rollover_time`` as a strict ``HH:MM`` - string, so a stored value never needs re-validation here; a missing key - falls back to :data:`~.const.DEFAULT_DAY_ROLLOVER_TIME`. + Thin shim over :class:`NestQuestSettings`: the options flow validates + ``day_rollover_time`` as a strict ``HH:MM`` string, so a stored value + never needs re-validation here; a missing key falls back to + :data:`~.const.DEFAULT_DAY_ROLLOVER_TIME`. """ - rollover = options.get(CONF_DAY_ROLLOVER_TIME, DEFAULT_DAY_ROLLOVER_TIME) - hour, _, minute = rollover.partition(":") - return int(hour), int(minute) + return NestQuestSettings.from_options(options).day_rollover_hour_minute def _configured_horizon_days(options: dict[str, Any]) -> int: """Return the configured generation horizon, validated, else the default. - The options flow already validates ``horizon_days`` as an integer >= 1, - but a stored ``entry.options`` dict could still carry a partial or - hand-edited value. Re-checking here — and falling back to - :data:`~.const.DEFAULT_HORIZON_DAYS` on a missing, non-int or sub-1 value - — keeps a malformed option from shrinking or exploding the materialized - horizon. + Thin shim over :class:`NestQuestSettings`. The settings object's + ``from_options`` re-checks ``horizon_days`` — rejecting bools, + non-ints, and sub-1 values, falling back to + :data:`~.const.DEFAULT_HORIZON_DAYS` — so a malformed option can + never shrink or explode the materialized horizon. """ - value = options.get(CONF_HORIZON_DAYS, DEFAULT_HORIZON_DAYS) - if isinstance(value, bool) or not isinstance(value, int) or value < 1: + try: + return NestQuestSettings.from_options(options).horizon_days + except ValueError: + # A malformed stored option (a bad time string) must not crash + # startup; the horizon itself was already validated above, so a + # ValueError here can only come from a sibling time field — fall + # back to the default horizon the same way the pre-settings path + # tolerated partial options. return DEFAULT_HORIZON_DAYS - return value async def _run_horizon_materialization( hass: HomeAssistant, database: NestQuestDatabase, - horizon_days: int = DEFAULT_HORIZON_DAYS, + settings: NestQuestSettings | None = None, + *, + horizon_days: int | None = None, ) -> None: """Materialize the rolling horizon ``[today, today + horizon_days]``. @@ -311,15 +315,22 @@ async def _run_horizon_materialization( This is the ONE generation path: the startup backfill, the daily rollover listener and the ``regenerate`` service all funnel through it, so there is - no duplicated materialization logic. ``horizon_days`` sizes the window - and defaults to :data:`~.const.DEFAULT_HORIZON_DAYS`; callers thread the - entry's validated option value through. + no duplicated materialization logic. Callers SHOULD pass a + :class:`NestQuestSettings` instance built from ``entry.options``; the + legacy ``horizon_days`` keyword is kept for callers that have not yet + been converted and overrides the settings value when supplied. When + neither is given the window sizes to + :data:`~.const.DEFAULT_HORIZON_DAYS`. """ + if settings is not None: + horizon = settings.horizon_days + else: + horizon = horizon_days if horizon_days is not None else DEFAULT_HORIZON_DAYS time_zone = ZoneInfo(hass.config.time_zone) today = datetime.datetime.now(time_zone).date() start_date = today.isoformat() end_date = ( - today + datetime.timedelta(days=horizon_days) + today + datetime.timedelta(days=horizon) ).isoformat() await _materialize_run(database, start_date, end_date, today=today) @@ -327,7 +338,7 @@ async def _run_horizon_materialization( def _register_day_rollover_listener( hass: HomeAssistant, database: NestQuestDatabase, - options: dict[str, Any], + settings: NestQuestSettings, ) -> Callable[[], Any]: """Register the daily rollover listener and return its remover. @@ -335,16 +346,15 @@ def _register_day_rollover_listener( first re-materializes the rolling horizon ``[today, today + horizon_days]`` through the shared :func:`_run_horizon_materialization` path (the entry's configured, - validated ``horizon_days``), then runs the Feature 11 missed sweep - — announcing yesterday's still-open quests as missed AFTER today's - instances exist, watermark-guarded so a re-fire of the listener the - same night announces nothing twice. + validated ``horizon_days`` carried on ``settings``), then runs the + Feature 11 missed sweep — announcing yesterday's still-open quests + as missed AFTER today's instances exist, watermark-guarded so a + re-fire of the listener the same night announces nothing twice. """ - hour, minute = _day_rollover_hour_minute(options) - horizon_days = _configured_horizon_days(options) + hour, minute = settings.day_rollover_hour_minute async def _run_materialization(_now: datetime.datetime) -> None: - await _run_horizon_materialization(hass, database, horizon_days) + await _run_horizon_materialization(hass, database, settings) await run_missed_sweep(hass, database) return async_track_time_change( @@ -379,9 +389,8 @@ def _make_regenerate_handler(hass: HomeAssistant): "live database; ignoring" ) return - horizon_days = _configured_horizon_days(runtime_data.options) await _run_horizon_materialization( - hass, runtime_data.database, horizon_days + hass, runtime_data.database, runtime_data.settings ) return _regenerate @@ -495,18 +504,26 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: context_user_id=context_user_id, owner_ids=owner_ids, ) + # Build the explicit settings object from entry.options ONCE: the + # horizon materialization and the daily-rollover listener both + # consume it, so core never reads HA config itself. A malformed + # stored option falls back to the defaults (the same way the + # pre-settings path tolerated a partial options dict) rather than + # crashing setup. + try: + settings = NestQuestSettings.from_options(entry.options) + except ValueError: + settings = NestQuestSettings() remove_update_listener = entry.add_update_listener(_async_update_listener) remove_time_change_listener = _register_day_rollover_listener( - hass, database, dict(entry.options) + hass, database, settings ) # Backfill any days the daily listener missed while the integration # was off (or freshly installed): run the one idempotent walk over # [today, today + horizon_days] once at setup, using the entry's # configured horizon. This runs through the SAME path as the daily # listener and regenerate service. - await _run_horizon_materialization( - hass, database, _configured_horizon_days(dict(entry.options)) - ) + await _run_horizon_materialization(hass, database, settings) # The missed sweep runs at startup too (Feature 11): if HA was # down at the rollover time, the previous night's still-open # quests are announced as missed now instead of waiting for @@ -530,6 +547,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: runtime_data = NestQuestRuntimeData( entry_id=entry.entry_id, options=dict(entry.options), + settings=settings, database=database, remove_update_listener=remove_update_listener, remove_time_change_listener=remove_time_change_listener, diff --git a/custom_components/nestquest/core/settings.py b/custom_components/nestquest/core/settings.py new file mode 100644 index 0000000..e1345a5 --- /dev/null +++ b/custom_components/nestquest/core/settings.py @@ -0,0 +1,302 @@ +"""Explicit, Home-Assistant-free settings object for NestQuest core. + +The integration reads its configuration off a Home Assistant config +entry (``entry.options``); the core package must not. This module +bridges that split: it defines a frozen, validation-carrying +:class:`NestQuestSettings` dataclass that the integration constructs +from ``entry.options`` (via :meth:`NestQuestSettings.from_options`) and +then passes into the core paths that previously read HA config +themselves — the rolling horizon (``horizon_days``) and the daily +rollover time (``day_rollover_time``), plus the Feature 11 notification +settings (notify target, the three summary/reminder/report times, and +the four independent enable toggles). + +Nothing in this module imports ``homeassistant`` or touches a config +entry; ``from_options`` accepts any plain mapping keyed by the +:data:`~.const.CONF_*` names. The validation semantics match the +options flow's: ``horizon_days`` must be a real integer >= 1 (bool +rejected), and ``day_rollover_time`` plus the three notification times +must be strict ``HH:MM`` 24-hour strings. Construction therefore can +never yield a settings object whose horizon could shrink or explode the +materialized window, or whose rollover fires at an unparsable time. +""" +from __future__ import annotations + +import datetime +import re +from dataclasses import dataclass +from types import MappingProxyType +from typing import Any, Mapping + +from .const import ( + CONF_AFTERNOON_REMINDER_ENABLED, + CONF_AFTERNOON_REMINDER_TIME, + CONF_CELEBRATION_ENABLED, + CONF_DAY_ROLLOVER_TIME, + CONF_END_OF_DAY_REPORT_ENABLED, + CONF_END_OF_DAY_REPORT_TIME, + CONF_HORIZON_DAYS, + CONF_MORNING_SUMMARY_ENABLED, + CONF_MORNING_SUMMARY_TIME, + CONF_NOTIFY_TARGET, + DEFAULT_AFTERNOON_REMINDER_TIME, + DEFAULT_AUTOMATION_ENABLED, + DEFAULT_DAY_ROLLOVER_TIME, + DEFAULT_END_OF_DAY_REPORT_TIME, + DEFAULT_HORIZON_DAYS, + DEFAULT_MORNING_SUMMARY_TIME, +) + +#: Strict 24-hour ``HH:MM`` pattern — matches the options flow's +#: ``_TIME_PATTERN`` verbatim so a stored option and a freshly-validated +#: form submit parse the same way. +_HHMM_PATTERN = re.compile(r"(?:[01]\d|2[0-3]):[0-5]\d") + + +def _option_value( + opts: Mapping[str, Any], key: str, default: Any +) -> Any: + """Return ``opts[key]`` when present and non-None, else ``default``. + + A stored ``None`` is treated the same as an absent key — the + historical ``horizon_days`` path mapped a missing-or-None value to + the default, and this keeps that forgiving semantics uniform across + every field: a partially-populated ``entry.options`` dict (or one + migrated across versions) never raises just because a key carries + ``None``. + """ + if key not in opts: + return default + value = opts[key] + return default if value is None else value + + +def _parse_hhmm(value: str) -> tuple[int, int]: + """Return ``(hour, minute)`` for a strict ``HH:MM`` string. + + Raises :class:`ValueError` for anything that is not a string or does + not match :data:`_HHMM_PATTERN` — the same rule the options flow + applies, surfaced here so a malformed mapping cannot construct a + settings object with an unparsable time. + """ + if not isinstance(value, str) or _HHMM_PATTERN.fullmatch(value) is None: + raise ValueError(f"expected a strict HH:MM string, got {value!r}") + hour_str, _, minute_str = value.partition(":") + return int(hour_str), int(minute_str) + + +def _resolve_horizon_days(value: Any) -> int: + """Validate a horizon value and return the resolved day count. + + Mirrors :func:`~.materialize._resolve_horizon_days`: a bool is + rejected (SQLite binds ``True``/``False`` onto an int), a non-int + is rejected, and a sub-1 value is rejected — so a malformed option + can never shrink or explode the materialized horizon. ``None`` + resolves to :data:`~.const.DEFAULT_HORIZON_DAYS`. + """ + if value is None: + return DEFAULT_HORIZON_DAYS + if isinstance(value, bool) or not isinstance(value, int) or value < 1: + raise ValueError( + f"horizon_days must be a positive integer, got {value!r}" + ) + return value + + +def _resolve_bool(value: Any, *, default: bool) -> bool: + """Return a real bool, falling back to ``default`` when ``value`` is None. + + A non-bool, non-None value is rejected — voluptuous ``bool`` would + coerce strings, so the options flow uses ``_strict_bool`` and this + mirrors it. + """ + if value is None: + return default + if not isinstance(value, bool): + raise ValueError(f"expected a boolean, got {value!r}") + return value + + +def _resolve_optional_str(value: Any) -> str: + """Return a stripped notify target string; empty means "not configured". + + A non-str value is rejected. An empty (or whitespace-only) value + is kept as the empty string — the shipped default, meaning no + notifications are configured yet — rather than raising, mirroring + the options flow's empty-target acceptance. + """ + if value is None: + return "" + if not isinstance(value, str): + raise ValueError(f"notify_target must be a string, got {value!r}") + return value.strip() + + +@dataclass(frozen=True) +class NestQuestSettings: + """Explicit, HA-free configuration carried into core paths. + + The integration builds this once from ``entry.options`` (via + :meth:`from_options`) and threads it into the horizon materialization + and the daily-rollover scheduling. Core never reads HA config + itself; callers pass the resolved horizon and rollover time in. + + All fields are validated at construction; the dataclass is frozen so + a stale settings object cannot be mutated in place after the + integration hands it off. + """ + + horizon_days: int = DEFAULT_HORIZON_DAYS + day_rollover_time: str = DEFAULT_DAY_ROLLOVER_TIME + notify_target: str = "" + morning_summary_time: str = DEFAULT_MORNING_SUMMARY_TIME + afternoon_reminder_time: str = DEFAULT_AFTERNOON_REMINDER_TIME + end_of_day_report_time: str = DEFAULT_END_OF_DAY_REPORT_TIME + morning_summary_enabled: bool = DEFAULT_AUTOMATION_ENABLED + afternoon_reminder_enabled: bool = DEFAULT_AUTOMATION_ENABLED + end_of_day_report_enabled: bool = DEFAULT_AUTOMATION_ENABLED + celebration_enabled: bool = DEFAULT_AUTOMATION_ENABLED + + def __post_init__(self) -> None: + # Validate at construction so a hand-built instance cannot carry + # an invalid value either: the same rules from_options applies. + _resolve_horizon_days(self.horizon_days) + _parse_hhmm(self.day_rollover_time) + if not isinstance(self.notify_target, str): + raise ValueError( + f"notify_target must be a string, got {self.notify_target!r}" + ) + _parse_hhmm(self.morning_summary_time) + _parse_hhmm(self.afternoon_reminder_time) + _parse_hhmm(self.end_of_day_report_time) + for name in ( + "morning_summary_enabled", + "afternoon_reminder_enabled", + "end_of_day_report_enabled", + "celebration_enabled", + ): + value = getattr(self, name) + if not isinstance(value, bool): + raise ValueError( + f"{name} must be a boolean, got {value!r}" + ) + + @property + def day_rollover_hour_minute(self) -> tuple[int, int]: + """The (hour, minute) the daily rollover fires at.""" + return _parse_hhmm(self.day_rollover_time) + + def horizon_window( + self, today: datetime.date | None = None + ) -> tuple[datetime.date, datetime.date]: + """Return the rolling horizon window ``[today, today + horizon_days]``. + + ``today`` defaults to the host clock's date + (:func:`datetime.date.today`), matching the historical materialize + default. The integration overrides this with the HA-local + ``hass.config.time_zone`` date so the horizon tracks the + household's own day. The returned window is closed/inclusive — + the same shape :func:`~.materialize.materialize` walks. + """ + if today is None: + today = datetime.date.today() + end = today + datetime.timedelta(days=self.horizon_days) + return today, end + + @classmethod + def from_options( + cls, options: Mapping[str, Any] | None = None + ) -> "NestQuestSettings": + """Build settings from a CONF_*-keyed mapping (e.g. ``entry.options``). + + Reads the same keys the options flow persists, applying the + matching defaults when a key is absent and validating the same + way the options flow does: ``horizon_days`` is a positive int + (bool rejected), the four times are strict ``HH:MM``, the four + toggles are real booleans, and ``notify_target`` is a string. + A ``None`` mapping yields the all-defaults settings — useful for + tests and for a fresh install whose options dict is empty. + + ``options`` is consumed read-only via + :class:`types.MappingProxyType`, so this method never mutates + the caller's mapping. + """ + opts: Mapping[str, Any] = ( + MappingProxyType({}) if options is None else MappingProxyType(options) + ) + return cls( + horizon_days=_resolve_horizon_days( + _option_value(opts, CONF_HORIZON_DAYS, DEFAULT_HORIZON_DAYS) + ), + day_rollover_time=_parse_hhmm_str( + _option_value(opts, CONF_DAY_ROLLOVER_TIME, DEFAULT_DAY_ROLLOVER_TIME), + field="day_rollover_time", + ), + notify_target=_resolve_optional_str( + _option_value(opts, CONF_NOTIFY_TARGET, "") + ), + morning_summary_time=_parse_hhmm_str( + _option_value( + opts, CONF_MORNING_SUMMARY_TIME, DEFAULT_MORNING_SUMMARY_TIME + ), + field="morning_summary_time", + ), + afternoon_reminder_time=_parse_hhmm_str( + _option_value( + opts, + CONF_AFTERNOON_REMINDER_TIME, + DEFAULT_AFTERNOON_REMINDER_TIME, + ), + field="afternoon_reminder_time", + ), + end_of_day_report_time=_parse_hhmm_str( + _option_value( + opts, + CONF_END_OF_DAY_REPORT_TIME, + DEFAULT_END_OF_DAY_REPORT_TIME, + ), + field="end_of_day_report_time", + ), + morning_summary_enabled=_resolve_bool( + _option_value( + opts, CONF_MORNING_SUMMARY_ENABLED, DEFAULT_AUTOMATION_ENABLED + ), + default=DEFAULT_AUTOMATION_ENABLED, + ), + afternoon_reminder_enabled=_resolve_bool( + _option_value( + opts, + CONF_AFTERNOON_REMINDER_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + ), + default=DEFAULT_AUTOMATION_ENABLED, + ), + end_of_day_report_enabled=_resolve_bool( + _option_value( + opts, + CONF_END_OF_DAY_REPORT_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + ), + default=DEFAULT_AUTOMATION_ENABLED, + ), + celebration_enabled=_resolve_bool( + _option_value( + opts, CONF_CELEBRATION_ENABLED, DEFAULT_AUTOMATION_ENABLED + ), + default=DEFAULT_AUTOMATION_ENABLED, + ), + ) + + +def _parse_hhmm_str(value: Any, *, field: str) -> str: + """Return a validated ``HH:MM`` string, raising on a malformed value. + + A separate helper from :func:`_parse_hhmm` because :meth:`from_options` + needs the original string kept (the dataclass stores strings, not + tuples), but still wants the same strict validation. + """ + if not isinstance(value, str) or _HHMM_PATTERN.fullmatch(value) is None: + raise ValueError( + f"{field} must be a strict HH:MM string, got {value!r}" + ) + return value diff --git a/custom_components/nestquest/settings.py b/custom_components/nestquest/settings.py new file mode 100644 index 0000000..9e8f62a --- /dev/null +++ b/custom_components/nestquest/settings.py @@ -0,0 +1,13 @@ +"""Re-export of :mod:`custom_components.nestquest.core.settings` for the integration package. + +The explicit, Home-Assistant-free settings object lives in +:mod:`custom_components.nestquest.core.settings` (introduced in task +4dd8f870); this module re-exports it so the integration constructs the +settings object from ``entry.options`` and threads it into the core +paths without core ever reading HA config itself. +""" +from __future__ import annotations + +from .core.settings import NestQuestSettings + +__all__ = ["NestQuestSettings"] diff --git a/tests/test_core_settings.py b/tests/test_core_settings.py new file mode 100644 index 0000000..070cc1c --- /dev/null +++ b/tests/test_core_settings.py @@ -0,0 +1,504 @@ +"""Tests for core/settings.py: the explicit, HA-free settings object. + +Covers three contracts from task 4dd8f870: + +- ``NestQuestSettings.from_options`` applies defaults when keys are + absent and rejects invalid horizon/rollover values. +- An AST scan asserts no module under + ``custom_components/nestquest/core/`` imports ``homeassistant``, + accesses a ``hass.config`` / config-entry attribute, or reads + config-entry options. (Docstring mentions are allowed.) +- A settings object constructed with NON-default ``horizon_days`` and + ``day_rollover_time`` drives a materialization whose resulting window + matches the non-default horizon. +""" +from __future__ import annotations + +import ast +import datetime +import inspect +from dataclasses import FrozenInstanceError +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import pytest + +from custom_components.nestquest.const import ( + CONF_AFTERNOON_REMINDER_ENABLED, + CONF_AFTERNOON_REMINDER_TIME, + CONF_CELEBRATION_ENABLED, + CONF_DAY_ROLLOVER_TIME, + CONF_END_OF_DAY_REPORT_ENABLED, + CONF_END_OF_DAY_REPORT_TIME, + CONF_HORIZON_DAYS, + CONF_MORNING_SUMMARY_ENABLED, + CONF_MORNING_SUMMARY_TIME, + CONF_NOTIFY_TARGET, + DEFAULT_AFTERNOON_REMINDER_TIME, + DEFAULT_AUTOMATION_ENABLED, + DEFAULT_DAY_ROLLOVER_TIME, + DEFAULT_END_OF_DAY_REPORT_TIME, + DEFAULT_HORIZON_DAYS, + DEFAULT_MORNING_SUMMARY_TIME, +) +from custom_components.nestquest.core.settings import NestQuestSettings + + +# --------------------------------------------------------------------------- +# from_options: defaults + validation. +# --------------------------------------------------------------------------- + + +def test_from_options_none_yields_all_defaults() -> None: + """A None mapping yields a settings object with every default applied.""" + s = NestQuestSettings.from_options(None) + assert s.horizon_days == DEFAULT_HORIZON_DAYS + assert s.day_rollover_time == DEFAULT_DAY_ROLLOVER_TIME + assert s.notify_target == "" + assert s.morning_summary_time == DEFAULT_MORNING_SUMMARY_TIME + assert s.afternoon_reminder_time == DEFAULT_AFTERNOON_REMINDER_TIME + assert s.end_of_day_report_time == DEFAULT_END_OF_DAY_REPORT_TIME + assert s.morning_summary_enabled is DEFAULT_AUTOMATION_ENABLED + assert s.afternoon_reminder_enabled is DEFAULT_AUTOMATION_ENABLED + assert s.end_of_day_report_enabled is DEFAULT_AUTOMATION_ENABLED + assert s.celebration_enabled is DEFAULT_AUTOMATION_ENABLED + + +def test_from_options_empty_mapping_yields_all_defaults() -> None: + """An empty mapping also yields every default (the fresh-install case).""" + s = NestQuestSettings.from_options({}) + assert s.horizon_days == DEFAULT_HORIZON_DAYS + assert s.day_rollover_time == DEFAULT_DAY_ROLLOVER_TIME + + +def test_from_options_reads_keys() -> None: + """Each CONF_* key is read into the matching field.""" + opts = { + CONF_HORIZON_DAYS: 21, + CONF_DAY_ROLLOVER_TIME: "03:30", + CONF_NOTIFY_TARGET: "notify.mobile_app_x", + CONF_MORNING_SUMMARY_TIME: "07:15", + CONF_AFTERNOON_REMINDER_TIME: "14:45", + CONF_END_OF_DAY_REPORT_TIME: "21:30", + CONF_MORNING_SUMMARY_ENABLED: False, + CONF_AFTERNOON_REMINDER_ENABLED: False, + CONF_END_OF_DAY_REPORT_ENABLED: False, + CONF_CELEBRATION_ENABLED: False, + } + s = NestQuestSettings.from_options(opts) + assert s.horizon_days == 21 + assert s.day_rollover_time == "03:30" + assert s.notify_target == "notify.mobile_app_x" + assert s.morning_summary_time == "07:15" + assert s.afternoon_reminder_time == "14:45" + assert s.end_of_day_report_time == "21:30" + assert s.morning_summary_enabled is False + assert s.afternoon_reminder_enabled is False + assert s.end_of_day_report_enabled is False + assert s.celebration_enabled is False + + +def test_from_options_does_not_mutate_caller_mapping() -> None: + """from_options consumes the mapping read-only.""" + opts: dict[str, Any] = {CONF_HORIZON_DAYS: 9} + snapshot = dict(opts) + _ = NestQuestSettings.from_options(opts) + assert opts == snapshot + + +@pytest.mark.parametrize( + "value", + [True, False, "5", 0, -3, 1.5], +) +def test_from_options_rejects_invalid_horizon(value: Any) -> None: + """A malformed horizon_days is rejected. + + A key that is ABSENT (or present-but-None) falls back to the + default — see :func:`test_from_options_horizon_absent_falls_back_to_default` + and the None case below; any other invalid value raises, matching + the options flow's strict validation. + """ + with pytest.raises(ValueError, match="horizon_days"): + NestQuestSettings.from_options({CONF_HORIZON_DAYS: value}) + + +def test_from_options_horizon_absent_falls_back_to_default() -> None: + """An absent (or None) horizon_days key falls back to the default.""" + assert NestQuestSettings.from_options({}).horizon_days == DEFAULT_HORIZON_DAYS + assert ( + NestQuestSettings.from_options( + {CONF_HORIZON_DAYS: None} + ).horizon_days + == DEFAULT_HORIZON_DAYS + ) + + +@pytest.mark.parametrize( + "value", + ["25:00", "00:60", "24:00", "9:5", "abc", "07:00:00", 700, ""], +) +def test_from_options_rejects_invalid_rollover(value: Any) -> None: + """A malformed day_rollover_time is rejected. + + A key that is ABSENT (or present-but-None) falls back to the + default — a present-but-empty-string or non-HH:MM value raises, + matching the options flow's strict validation. + """ + with pytest.raises(ValueError): + NestQuestSettings.from_options({CONF_DAY_ROLLOVER_TIME: value}) + + +def test_from_options_rollover_absent_falls_back_to_default() -> None: + """An absent (or None) day_rollover_time key falls back to the default.""" + assert ( + NestQuestSettings.from_options({}).day_rollover_time + == DEFAULT_DAY_ROLLOVER_TIME + ) + assert ( + NestQuestSettings.from_options( + {CONF_DAY_ROLLOVER_TIME: None} + ).day_rollover_time + == DEFAULT_DAY_ROLLOVER_TIME + ) + + +@pytest.mark.parametrize( + "key", + [ + CONF_MORNING_SUMMARY_TIME, + CONF_AFTERNOON_REMINDER_TIME, + CONF_END_OF_DAY_REPORT_TIME, + ], +) +def test_from_options_rejects_invalid_notification_time(key: str) -> None: + """The three notification times are validated as strict HH:MM.""" + with pytest.raises(ValueError): + NestQuestSettings.from_options({key: "25:99"}) + + +@pytest.mark.parametrize( + "key", + [ + CONF_MORNING_SUMMARY_ENABLED, + CONF_AFTERNOON_REMINDER_ENABLED, + CONF_END_OF_DAY_REPORT_ENABLED, + CONF_CELEBRATION_ENABLED, + ], +) +def test_from_options_rejects_non_bool_toggle(key: str) -> None: + """Each toggle must be a real bool (no string coercion).""" + with pytest.raises(ValueError): + NestQuestSettings.from_options({key: "yes"}) + + +def test_from_options_rejects_non_string_notify_target() -> None: + """notify_target must be a string (or absent).""" + with pytest.raises(ValueError): + NestQuestSettings.from_options({CONF_NOTIFY_TARGET: 123}) + + +def test_from_options_strips_notify_target() -> None: + """A padded notify target is stripped; empty stays empty.""" + s = NestQuestSettings.from_options({CONF_NOTIFY_TARGET: " notify.x "}) + assert s.notify_target == "notify.x" + s_empty = NestQuestSettings.from_options({CONF_NOTIFY_TARGET: " "}) + assert s_empty.notify_target == "" + + +# --------------------------------------------------------------------------- +# Construction + frozenness. +# --------------------------------------------------------------------------- + + +def test_direct_construction_validates() -> None: + """Hand-built instances validate the same way from_options does.""" + with pytest.raises(ValueError): + NestQuestSettings(horizon_days=0) + with pytest.raises(ValueError): + NestQuestSettings(day_rollover_time="bad") + with pytest.raises(ValueError): + NestQuestSettings(morning_summary_enabled="yes") + + +def test_settings_is_frozen() -> None: + """The dataclass is frozen so a stale settings object cannot be mutated.""" + s = NestQuestSettings() + with pytest.raises(FrozenInstanceError): + s.horizon_days = 99 # type: ignore[misc] + + +def test_day_rollover_hour_minute() -> None: + """day_rollover_hour_minute returns the parsed (hour, minute).""" + s = NestQuestSettings(day_rollover_time="03:30") + assert s.day_rollover_hour_minute == (3, 30) + assert NestQuestSettings().day_rollover_hour_minute == (0, 0) + + +# --------------------------------------------------------------------------- +# horizon_window. +# --------------------------------------------------------------------------- + + +def test_horizon_window_default_today() -> None: + """horizon_window returns [today, today + horizon_days] when today is omitted.""" + s = NestQuestSettings(horizon_days=7) + start, end = s.horizon_window() + today = datetime.date.today() + assert start == today + assert end == today + datetime.timedelta(days=7) + + +def test_horizon_window_pinned_today() -> None: + """A pinned today is honored so the window is deterministic.""" + s = NestQuestSettings(horizon_days=5) + pinned = datetime.date(2026, 3, 1) + start, end = s.horizon_window(pinned) + assert start == pinned + assert end == pinned + datetime.timedelta(days=5) + + +def test_horizon_window_non_default_horizon() -> None: + """A non-default horizon_days widens the window accordingly.""" + pinned = datetime.date(2026, 3, 1) + short = NestQuestSettings(horizon_days=3).horizon_window(pinned) + long_ = NestQuestSettings(horizon_days=30).horizon_window(pinned) + assert short[1] == pinned + datetime.timedelta(days=3) + assert long_[1] == pinned + datetime.timedelta(days=30) + assert (long_[1] - short[1]).days == 27 + + +# --------------------------------------------------------------------------- +# AST: no HA config reads under core/. +# --------------------------------------------------------------------------- + +CORE_PKG = Path( + __import__( + "custom_components.nestquest.core", fromlist=["__file__"] + ).__file__ +).parent + + +def _is_homeassistant(module: str) -> bool: + return module == "homeassistant" or module.startswith("homeassistant.") + + +def _attribute_chain(node: ast.Attribute) -> list[str]: + """Flatten an attribute access chain into dotted-name parts (leaf first).""" + parts: list[str] = [node.attr] + cur: ast.expr = node.value + while isinstance(cur, ast.Attribute): + parts.append(cur.attr) + cur = cur.value + if isinstance(cur, ast.Name): + parts.append(cur.id) + return parts + + +def _dotted_attr(node: ast.Attribute) -> str: + """Return the dotted name of an attribute chain, root first.""" + return ".".join(reversed(_attribute_chain(node))) + + +def _scan_module_for_ha_config(path: Path) -> list[str]: + """Return a list of offender strings for HA-coupling in ``path``. + + Flags: + - any ``import homeassistant`` / ``from homeassistant ...``; + - any attribute access whose chain reaches ``hass.config`` or any + ``.options``/``.data`` access on a name literally named ``entry`` + (a config-entry attribute read), EXCEPT inside a docstring-only + module (no runtime access) — but since AST only sees code, not + docstrings, docstring mentions are structurally exempt. + - any call to ``.options.get(...)`` / ``.data.get(...)`` on a name + literally named ``entry`` (a config-entry options/data read). + """ + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + offenders: list[str] = [] + + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + if _is_homeassistant(alias.name): + offenders.append( + f"{path.name}:{node.lineno} homeassistant import: " + f"import {alias.name}" + ) + elif isinstance(node, ast.ImportFrom): + base = node.module or "" + if _is_homeassistant(base): + offenders.append( + f"{path.name}:{node.lineno} homeassistant import: from {base}" + ) + for alias in node.names: + combined = ( + f"{base}.{alias.name}" if base else alias.name + ) + if _is_homeassistant(combined): + offenders.append( + f"{path.name}:{node.lineno} homeassistant import: " + f"from {base} import {alias.name}" + ) + elif isinstance(node, ast.Attribute): + dotted = _dotted_attr(node) + # hass.config. — a HA config read. + if dotted == "hass.config" or dotted.startswith("hass.config."): + offenders.append( + f"{path.name}:{node.lineno} hass.config access: {dotted}" + ) + # entry.options / entry.data — a config-entry attribute read. + # Only flag the leaf attribute itself (not every sub-expression), + # and only when the root name is literally 'entry'. + root = _attribute_chain(node)[-1] + if root == "entry" and node.attr in {"options", "data"}: + offenders.append( + f"{path.name}:{node.lineno} config-entry attribute read: " + f"{dotted}" + ) + + return offenders + + +def test_core_modules_have_no_ha_config_reads() -> None: + """No core module imports homeassistant or reads HA config / entry options. + + Docstring mentions are allowed: AST only sees code, and the offenders + this scan flags (imports, attribute accesses) never appear in a + docstring. The existing docstring references to ``hass.config`` in + ``core/events.py`` and ``core/store.py`` therefore pass. + """ + py_files = sorted(CORE_PKG.rglob("*.py")) + assert py_files, "core package not found" + all_offenders: list[str] = [] + for py_file in py_files: + all_offenders.extend(_scan_module_for_ha_config(py_file)) + assert all_offenders == [], ( + "core package reads HA config / entry options:\n" + + "\n".join(all_offenders) + ) + + +def test_settings_module_lives_under_core() -> None: + """settings.py is part of the core package (not the integration root).""" + assert (CORE_PKG / "settings.py").is_file() + # And it is importable as a core submodule. + import custom_components.nestquest.core.settings as mod + + assert hasattr(mod, "NestQuestSettings") + assert inspect.isclass(mod.NestQuestSettings) + + +def test_settings_module_has_no_ha_imports() -> None: + """settings.py specifically carries no homeassistant import.""" + offenders = _scan_module_for_ha_config(CORE_PKG / "settings.py") + assert offenders == [], offenders + + +# --------------------------------------------------------------------------- +# Materialization over a non-default horizon. +# --------------------------------------------------------------------------- + + +def _make_hass_mock(): + from unittest.mock import AsyncMock + + hass = MagicMock() + hass.async_add_executor_job = AsyncMock( + side_effect=(lambda fn, *a, **k: fn(*a, **k)) + ) + return hass + + +async def _prepare(path): + from custom_components.nestquest.core.db import NestQuestDatabase + from custom_components.nestquest.core.migrations import apply_migrations + + database = NestQuestDatabase(_make_hass_mock().async_add_executor_job) + await database.open(path) + await apply_migrations(database) + return database + + +def _run(coro): + import asyncio + + return asyncio.new_event_loop().run_until_complete(coro) + + +def test_materialize_over_non_default_horizon(tmp_path) -> None: + """A settings object with a non-default horizon drives the matching window. + + Constructs NestQuestSettings with horizon_days=3 and day_rollover_time + '03:30' (both non-default), builds the window via horizon_window, and + drives materialize over it; asserts the resulting instance rows land + exactly on the [today, today+3] window and NOT beyond it. + """ + import asyncio + + from custom_components.nestquest.core.dao_children import ChildrenDao + from custom_components.nestquest.core.dao_instances import ( + QuestInstancesDao, + ) + from custom_components.nestquest.core.materialize import materialize + from custom_components.nestquest.core.quest_definitions import ( + create_quest_definition, + ) + from custom_components.nestquest.core.recurrence import ( + RuleType, + ScheduleRule, + ) + + async def _main(): + database = await _prepare(tmp_path / "nq.db") + try: + settings = NestQuestSettings( + horizon_days=3, day_rollover_time="03:30" + ) + assert settings.horizon_days == 3 + assert settings.day_rollover_time == "03:30" + + today = datetime.date(2026, 3, 1) + start, end = settings.horizon_window(today) + assert start == today + assert end == today + datetime.timedelta(days=3) + + NOW = "2026-03-01T12:00:00+00:00" + child = await ChildrenDao(database).create("Ada", NOW) + await create_quest_definition( + database, + "Daily chore", + ScheduleRule( + rule_type=RuleType.DAILY, start_date=today.isoformat() + ), + [child.id], + ["morning"], + ) + + count = await materialize( + database, + start.isoformat(), + end.isoformat(), + today=today, + ) + # 4 days (today..today+3 inclusive), one instance per day. + assert count == 4 + + records = await QuestInstancesDao(database).list_by_date_range( + child.id, start.isoformat(), end.isoformat() + ) + dates = sorted({r.due_date for r in records}) + assert dates == [ + (today + datetime.timedelta(days=i)).isoformat() + for i in range(4) + ] + + # Beyond the configured horizon: no instances. + beyond = (end + datetime.timedelta(days=1)).isoformat() + far = (end + datetime.timedelta(days=10)).isoformat() + assert await QuestInstancesDao(database).list_by_date_range( + child.id, beyond, far + ) == [] + finally: + await database.close() + + asyncio.new_event_loop().run_until_complete(_main()) -- 2.55.0 From 33102f8806ef3965006fd92d821c35ef31a3dbec Mon Sep 17 00:00:00 2001 From: Joshua Date: Mon, 21 Sep 2026 15:37:12 -0400 Subject: [PATCH 2/4] Apply PR #144 review fixes: per-field settings fallback, strict validation, hardened AST scan (task 4dd8f870) P2-1: replace the all-or-nothing from_options try/except in setup with from_options_resilient, which falls back PER FIELD (logging a warning naming the bad field) so a malformed sibling (e.g. morning_summary_time '25:00') cannot silently reset horizon_days/day_rollover_time. P2-2: drop _run_horizon_materialization's legacy horizon_days keyword and branch; default settings to NestQuestSettings(); remove the legacy paragraph. P2-3: _run_horizon_materialization now calls settings.horizon_window(today) so the helper is real and shared; extend tests to run the integration path (_run_horizon_materialization + _register_day_rollover_listener) with horizon_days=3 / day_rollover_time='03:30'. P2-4: harden the AST scan to forbid hass/config_entry names, .hass/.entry/ .config_entry and .config/.options/.data attributes, and getattr/hasattr calls with those string args; add a negative-control test for each shape. P2-5: __post_init__ is strict (rejects None for every field); the None->default mapping lives ONLY in from_options via _option_value. P3: remove dead shims (_day_rollover_hour_minute, _configured_horizon_days) and repoint test_lifecycle at from_options; de-dup the two HH:MM validators into one using a shared TIME_PATTERN in core/const; make horizon_window's today required; reword the notify_target docstring to not claim options-flow parity; document _option_value's None-as-absent semantics; remove unused test helpers. --- custom_components/nestquest/__init__.py | 71 +-- custom_components/nestquest/core/const.py | 8 + custom_components/nestquest/core/settings.py | 319 +++++++--- custom_components/nestquest/options_flow.py | 6 +- tests/test_core_settings.py | 596 ++++++++++++++----- tests/test_lifecycle.py | 29 +- 6 files changed, 718 insertions(+), 311 deletions(-) diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index abea786..5bfea62 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -19,7 +19,6 @@ from homeassistant.helpers.event import async_track_time_change from .const import ( CONF_ADMIN_USER_IDS, CONF_UPDATE_INTERVAL, - DEFAULT_HORIZON_DAYS, DEFAULT_UPDATE_INTERVAL, DOMAIN, LOGGER, @@ -268,43 +267,10 @@ async def _async_owner_user_ids(hass: HomeAssistant) -> list[str]: return owners -def _day_rollover_hour_minute(options: dict[str, Any]) -> tuple[int, int]: - """Return the (hour, minute) the daily rollover fires at from ``options``. - - Thin shim over :class:`NestQuestSettings`: the options flow validates - ``day_rollover_time`` as a strict ``HH:MM`` string, so a stored value - never needs re-validation here; a missing key falls back to - :data:`~.const.DEFAULT_DAY_ROLLOVER_TIME`. - """ - return NestQuestSettings.from_options(options).day_rollover_hour_minute - - -def _configured_horizon_days(options: dict[str, Any]) -> int: - """Return the configured generation horizon, validated, else the default. - - Thin shim over :class:`NestQuestSettings`. The settings object's - ``from_options`` re-checks ``horizon_days`` — rejecting bools, - non-ints, and sub-1 values, falling back to - :data:`~.const.DEFAULT_HORIZON_DAYS` — so a malformed option can - never shrink or explode the materialized horizon. - """ - try: - return NestQuestSettings.from_options(options).horizon_days - except ValueError: - # A malformed stored option (a bad time string) must not crash - # startup; the horizon itself was already validated above, so a - # ValueError here can only come from a sibling time field — fall - # back to the default horizon the same way the pre-settings path - # tolerated partial options. - return DEFAULT_HORIZON_DAYS - - async def _run_horizon_materialization( hass: HomeAssistant, database: NestQuestDatabase, settings: NestQuestSettings | None = None, - *, - horizon_days: int | None = None, ) -> None: """Materialize the rolling horizon ``[today, today + horizon_days]``. @@ -315,24 +281,18 @@ async def _run_horizon_materialization( This is the ONE generation path: the startup backfill, the daily rollover listener and the ``regenerate`` service all funnel through it, so there is - no duplicated materialization logic. Callers SHOULD pass a - :class:`NestQuestSettings` instance built from ``entry.options``; the - legacy ``horizon_days`` keyword is kept for callers that have not yet - been converted and overrides the settings value when supplied. When - neither is given the window sizes to - :data:`~.const.DEFAULT_HORIZON_DAYS`. + no duplicated materialization logic. ``settings`` carries the entry's + validated ``horizon_days``; when omitted a default + :class:`NestQuestSettings` is used (its horizon is the configured + default). The window itself comes from :meth:`settings.horizon_window` + so the helper is real and shared (not recomputed inline here). """ - if settings is not None: - horizon = settings.horizon_days - else: - horizon = horizon_days if horizon_days is not None else DEFAULT_HORIZON_DAYS + if settings is None: + settings = NestQuestSettings() time_zone = ZoneInfo(hass.config.time_zone) today = datetime.datetime.now(time_zone).date() - start_date = today.isoformat() - end_date = ( - today + datetime.timedelta(days=horizon) - ).isoformat() - await _materialize_run(database, start_date, end_date, today=today) + start, end = settings.horizon_window(today) + await _materialize_run(database, start.isoformat(), end.isoformat(), today=today) def _register_day_rollover_listener( @@ -507,13 +467,12 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: # Build the explicit settings object from entry.options ONCE: the # horizon materialization and the daily-rollover listener both # consume it, so core never reads HA config itself. A malformed - # stored option falls back to the defaults (the same way the - # pre-settings path tolerated a partial options dict) rather than - # crashing setup. - try: - settings = NestQuestSettings.from_options(entry.options) - except ValueError: - settings = NestQuestSettings() + # stored option falls back PER FIELD (via from_options_resilient) + # so a single bad sibling option cannot silently reset the user's + # horizon_days or day_rollover_time — the two fields core consumes + # — rather than the all-or-nothing reset a single try/except would + # cause (a malformed morning_summary_time used to wipe horizon_days). + settings = NestQuestSettings.from_options_resilient(entry.options) remove_update_listener = entry.add_update_listener(_async_update_listener) remove_time_change_listener = _register_day_rollover_listener( hass, database, settings diff --git a/custom_components/nestquest/core/const.py b/custom_components/nestquest/core/const.py index e81d78e..fc72aff 100644 --- a/custom_components/nestquest/core/const.py +++ b/custom_components/nestquest/core/const.py @@ -2,11 +2,19 @@ from __future__ import annotations import logging +import re DOMAIN = "nestquest" LOGGER_NAME = "custom_components.nestquest" LOGGER = logging.getLogger(LOGGER_NAME) +#: Strict 24-hour ``HH:MM`` pattern shared by the options flow and the +#: settings object's time validation, so a stored option and a freshly +#: validated form submit parse the same way. Defined here (HA-free) so +#: both the integration's options flow and the core settings module +#: import ONE pattern instead of re-copying it. +TIME_PATTERN = re.compile(r"(?:[01]\d|2[0-3]):[0-5]\d") + # Platforms set up per config entry (Feature 10): the per-child day # sensors and binary sensors. PLATFORMS: list[str] = ["sensor", "binary_sensor"] diff --git a/custom_components/nestquest/core/settings.py b/custom_components/nestquest/core/settings.py index e1345a5..25d5e57 100644 --- a/custom_components/nestquest/core/settings.py +++ b/custom_components/nestquest/core/settings.py @@ -4,12 +4,13 @@ The integration reads its configuration off a Home Assistant config entry (``entry.options``); the core package must not. This module bridges that split: it defines a frozen, validation-carrying :class:`NestQuestSettings` dataclass that the integration constructs -from ``entry.options`` (via :meth:`NestQuestSettings.from_options`) and -then passes into the core paths that previously read HA config -themselves — the rolling horizon (``horizon_days``) and the daily -rollover time (``day_rollover_time``), plus the Feature 11 notification -settings (notify target, the three summary/reminder/report times, and -the four independent enable toggles). +from ``entry.options`` (via :meth:`NestQuestSettings.from_options`, or +:meth:`NestQuestSettings.from_options_resilient` for the per-field +fallback used at setup) and then passes into the core paths that +previously read HA config themselves — the rolling horizon +(``horizon_days``) and the daily rollover time (``day_rollover_time``), +plus the Feature 11 notification settings (notify target, the three +summary/reminder/report times, and the four independent enable toggles). Nothing in this module imports ``homeassistant`` or touches a config entry; ``from_options`` accepts any plain mapping keyed by the @@ -19,11 +20,18 @@ rejected), and ``day_rollover_time`` plus the three notification times must be strict ``HH:MM`` 24-hour strings. Construction therefore can never yield a settings object whose horizon could shrink or explode the materialized window, or whose rollover fires at an unparsable time. + +The strict resolvers (:func:`_resolve_horizon_days`, :func:`_validate_hhmm`, +:func:`_resolve_bool`, :func:`_resolve_optional_str`) reject ``None``; the +``None``-as-absent mapping lives ONLY in :meth:`from_options` / +:meth:`from_options_resilient` (via :func:`_option_value`), so a hand-built +:class:`NestQuestSettings` cannot smuggle a ``None`` past +:meth:`__post_init__`. """ from __future__ import annotations import datetime -import re +from collections.abc import Callable from dataclasses import dataclass from types import MappingProxyType from typing import Any, Mapping @@ -45,25 +53,27 @@ from .const import ( DEFAULT_END_OF_DAY_REPORT_TIME, DEFAULT_HORIZON_DAYS, DEFAULT_MORNING_SUMMARY_TIME, + LOGGER, + TIME_PATTERN, ) -#: Strict 24-hour ``HH:MM`` pattern — matches the options flow's -#: ``_TIME_PATTERN`` verbatim so a stored option and a freshly-validated -#: form submit parse the same way. -_HHMM_PATTERN = re.compile(r"(?:[01]\d|2[0-3]):[0-5]\d") - def _option_value( opts: Mapping[str, Any], key: str, default: Any ) -> Any: """Return ``opts[key]`` when present and non-None, else ``default``. - A stored ``None`` is treated the same as an absent key — the - historical ``horizon_days`` path mapped a missing-or-None value to - the default, and this keeps that forgiving semantics uniform across - every field: a partially-populated ``entry.options`` dict (or one - migrated across versions) never raises just because a key carries - ``None``. + This is the SOLE place the ``None``-as-absent mapping lives: a stored + ``None`` is treated the same as an absent key — the historical + ``horizon_days`` path mapped a missing-or-None value to the default, + and this keeps that forgiving semantics uniform across every field. + The per-field resolvers (:func:`_resolve_horizon_days`, + :func:`_validate_hhmm`, :func:`_resolve_bool`, + :func:`_resolve_optional_str`) are STRICT — they reject ``None`` — + so a partially-populated ``entry.options`` dict (or one migrated + across versions) never raises just because a key carries ``None``, + and a hand-built :class:`NestQuestSettings` cannot smuggle a + ``None`` past :meth:`NestQuestSettings.__post_init__`. """ if key not in opts: return default @@ -71,31 +81,36 @@ def _option_value( return default if value is None else value -def _parse_hhmm(value: str) -> tuple[int, int]: - """Return ``(hour, minute)`` for a strict ``HH:MM`` string. +def _validate_hhmm(value: Any, *, field: str) -> str: + """Validate a strict ``HH:MM`` string and return it; raise naming ``field``. - Raises :class:`ValueError` for anything that is not a string or does - not match :data:`_HHMM_PATTERN` — the same rule the options flow - applies, surfaced here so a malformed mapping cannot construct a - settings object with an unparsable time. + Uses the shared :data:`~.const.TIME_PATTERN` (the same compiled + regex the options flow applies), so a stored option and a freshly + validated form submit parse the same way. ``None`` (and any + non-string) is rejected; the ``None``-as-absent mapping lives ONLY + in :meth:`NestQuestSettings.from_options` (via :func:`_option_value`). """ - if not isinstance(value, str) or _HHMM_PATTERN.fullmatch(value) is None: - raise ValueError(f"expected a strict HH:MM string, got {value!r}") - hour_str, _, minute_str = value.partition(":") - return int(hour_str), int(minute_str) + if not isinstance(value, str) or TIME_PATTERN.fullmatch(value) is None: + raise ValueError( + f"{field} must be a strict HH:MM string, got {value!r}" + ) + return value def _resolve_horizon_days(value: Any) -> int: """Validate a horizon value and return the resolved day count. - Mirrors :func:`~.materialize._resolve_horizon_days`: a bool is - rejected (SQLite binds ``True``/``False`` onto an int), a non-int - is rejected, and a sub-1 value is rejected — so a malformed option - can never shrink or explode the materialized horizon. ``None`` - resolves to :data:`~.const.DEFAULT_HORIZON_DAYS`. + A bool is rejected (SQLite binds ``True``/``False`` onto an int), a + non-int is rejected, and a sub-1 value is rejected — so a malformed + option can never shrink or explode the materialized horizon. + ``None`` is rejected here too; the ``None``-as-absent mapping lives + ONLY in :meth:`NestQuestSettings.from_options` (via + :func:`_option_value`), so a hand-built + :class:`NestQuestSettings(horizon_days=None)` raises in + :meth:`NestQuestSettings.__post_init__` rather than silently + constructing an object whose :meth:`NestQuestSettings.horizon_window` + would later raise ``TypeError`` on ``timedelta(days=None)``. """ - if value is None: - return DEFAULT_HORIZON_DAYS if isinstance(value, bool) or not isinstance(value, int) or value < 1: raise ValueError( f"horizon_days must be a positive integer, got {value!r}" @@ -103,30 +118,32 @@ def _resolve_horizon_days(value: Any) -> int: return value -def _resolve_bool(value: Any, *, default: bool) -> bool: - """Return a real bool, falling back to ``default`` when ``value`` is None. +def _resolve_bool(value: Any) -> bool: + """Return a real bool; a non-bool (including ``None``) is rejected. - A non-bool, non-None value is rejected — voluptuous ``bool`` would - coerce strings, so the options flow uses ``_strict_bool`` and this - mirrors it. + voluptuous ``bool`` would coerce strings, so the options flow uses a + strict-bool validator and this mirrors it. The ``None``-as-absent + mapping lives ONLY in :meth:`NestQuestSettings.from_options` (via + :func:`_option_value`). """ - if value is None: - return default if not isinstance(value, bool): raise ValueError(f"expected a boolean, got {value!r}") return value def _resolve_optional_str(value: Any) -> str: - """Return a stripped notify target string; empty means "not configured". + """Return a stripped notify target string; a non-str is rejected. - A non-str value is rejected. An empty (or whitespace-only) value - is kept as the empty string — the shipped default, meaning no - notifications are configured yet — rather than raising, mirroring - the options flow's empty-target acceptance. + A padded or whitespace-only value is STRIPPED to its trim — the + shipped default is the empty string, meaning "no notifications are + configured yet". This is intentionally MORE LENIENT than the options + flow, which REJECTS a target that differs from its own trim (so a + padded value never persists): the integration tolerates a stored + padded value (e.g. one migrated from an older schema) by normalizing + it rather than crashing setup. The ``None``-as-absent mapping lives + ONLY in :meth:`NestQuestSettings.from_options` (via + :func:`_option_value`). """ - if value is None: - return "" if not isinstance(value, str): raise ValueError(f"notify_target must be a string, got {value!r}") return value.strip() @@ -137,9 +154,11 @@ class NestQuestSettings: """Explicit, HA-free configuration carried into core paths. The integration builds this once from ``entry.options`` (via - :meth:`from_options`) and threads it into the horizon materialization - and the daily-rollover scheduling. Core never reads HA config - itself; callers pass the resolved horizon and rollover time in. + :meth:`from_options`, or :meth:`from_options_resilient` for the + per-field fallback used at setup) and threads it into the horizon + materialization and the daily-rollover scheduling. Core never + reads HA config itself; callers pass the resolved horizon and + rollover time in. All fields are validated at construction; the dataclass is frozen so a stale settings object cannot be mutated in place after the @@ -158,17 +177,25 @@ class NestQuestSettings: celebration_enabled: bool = DEFAULT_AUTOMATION_ENABLED def __post_init__(self) -> None: - # Validate at construction so a hand-built instance cannot carry - # an invalid value either: the same rules from_options applies. + # Validate STRICTLY at construction so a hand-built instance + # cannot carry an invalid value: the same rules from_options + # applies, but WITHOUT the None->default mapping (that lives + # only in from_options via _option_value). A None horizon, time, + # toggle, or notify target raises here rather than later blowing + # up horizon_window or the rollover listener. _resolve_horizon_days(self.horizon_days) - _parse_hhmm(self.day_rollover_time) + _validate_hhmm(self.day_rollover_time, field="day_rollover_time") if not isinstance(self.notify_target, str): raise ValueError( f"notify_target must be a string, got {self.notify_target!r}" ) - _parse_hhmm(self.morning_summary_time) - _parse_hhmm(self.afternoon_reminder_time) - _parse_hhmm(self.end_of_day_report_time) + _validate_hhmm(self.morning_summary_time, field="morning_summary_time") + _validate_hhmm( + self.afternoon_reminder_time, field="afternoon_reminder_time" + ) + _validate_hhmm( + self.end_of_day_report_time, field="end_of_day_report_time" + ) for name in ( "morning_summary_enabled", "afternoon_reminder_enabled", @@ -184,22 +211,25 @@ class NestQuestSettings: @property def day_rollover_hour_minute(self) -> tuple[int, int]: """The (hour, minute) the daily rollover fires at.""" - return _parse_hhmm(self.day_rollover_time) + # The string is validated at construction, so a plain partition + # is safe and avoids re-running the regex on every access. + hour_str, _, minute_str = self.day_rollover_time.partition(":") + return int(hour_str), int(minute_str) def horizon_window( - self, today: datetime.date | None = None + self, today: datetime.date ) -> tuple[datetime.date, datetime.date]: """Return the rolling horizon window ``[today, today + horizon_days]``. - ``today`` defaults to the host clock's date - (:func:`datetime.date.today`), matching the historical materialize - default. The integration overrides this with the HA-local - ``hass.config.time_zone`` date so the horizon tracks the - household's own day. The returned window is closed/inclusive — - the same shape :func:`~.materialize.materialize` walks. + ``today`` is REQUIRED (there is no host-clock default): the + integration passes the HA-local ``hass.config.time_zone`` date so + the horizon tracks the household's own day, and a silent + host-clock fallback would be a wrong-day footgun (a test or a + caller that forgets to pass ``today`` would otherwise materialize + against the machine's calendar, not the household's). The + returned window is closed/inclusive — the same shape + :func:`~.materialize.materialize` walks. """ - if today is None: - today = datetime.date.today() end = today + datetime.timedelta(days=self.horizon_days) return today, end @@ -210,12 +240,16 @@ class NestQuestSettings: """Build settings from a CONF_*-keyed mapping (e.g. ``entry.options``). Reads the same keys the options flow persists, applying the - matching defaults when a key is absent and validating the same - way the options flow does: ``horizon_days`` is a positive int - (bool rejected), the four times are strict ``HH:MM``, the four - toggles are real booleans, and ``notify_target`` is a string. - A ``None`` mapping yields the all-defaults settings — useful for - tests and for a fresh install whose options dict is empty. + matching defaults when a key is absent OR ``None`` (via + :func:`_option_value`, the SOLE place the ``None``-as-absent + mapping lives) and validating STRICTLY: ``horizon_days`` is a + positive int (bool rejected), the four times are strict + ``HH:MM``, the four toggles are real booleans, and + ``notify_target`` is a string (stripped). A malformed value + RAISES — the integration uses :meth:`from_options_resilient` at + setup to fall back per field instead of crashing. A ``None`` + mapping yields the all-defaults settings — useful for tests and + for a fresh install whose options dict is empty. ``options`` is consumed read-only via :class:`types.MappingProxyType`, so this method never mutates @@ -228,20 +262,24 @@ class NestQuestSettings: horizon_days=_resolve_horizon_days( _option_value(opts, CONF_HORIZON_DAYS, DEFAULT_HORIZON_DAYS) ), - day_rollover_time=_parse_hhmm_str( - _option_value(opts, CONF_DAY_ROLLOVER_TIME, DEFAULT_DAY_ROLLOVER_TIME), + day_rollover_time=_validate_hhmm( + _option_value( + opts, CONF_DAY_ROLLOVER_TIME, DEFAULT_DAY_ROLLOVER_TIME + ), field="day_rollover_time", ), notify_target=_resolve_optional_str( _option_value(opts, CONF_NOTIFY_TARGET, "") ), - morning_summary_time=_parse_hhmm_str( + morning_summary_time=_validate_hhmm( _option_value( - opts, CONF_MORNING_SUMMARY_TIME, DEFAULT_MORNING_SUMMARY_TIME + opts, + CONF_MORNING_SUMMARY_TIME, + DEFAULT_MORNING_SUMMARY_TIME, ), field="morning_summary_time", ), - afternoon_reminder_time=_parse_hhmm_str( + afternoon_reminder_time=_validate_hhmm( _option_value( opts, CONF_AFTERNOON_REMINDER_TIME, @@ -249,7 +287,7 @@ class NestQuestSettings: ), field="afternoon_reminder_time", ), - end_of_day_report_time=_parse_hhmm_str( + end_of_day_report_time=_validate_hhmm( _option_value( opts, CONF_END_OF_DAY_REPORT_TIME, @@ -260,43 +298,128 @@ class NestQuestSettings: morning_summary_enabled=_resolve_bool( _option_value( opts, CONF_MORNING_SUMMARY_ENABLED, DEFAULT_AUTOMATION_ENABLED - ), - default=DEFAULT_AUTOMATION_ENABLED, + ) ), afternoon_reminder_enabled=_resolve_bool( _option_value( opts, CONF_AFTERNOON_REMINDER_ENABLED, DEFAULT_AUTOMATION_ENABLED, - ), - default=DEFAULT_AUTOMATION_ENABLED, + ) ), end_of_day_report_enabled=_resolve_bool( _option_value( opts, CONF_END_OF_DAY_REPORT_ENABLED, DEFAULT_AUTOMATION_ENABLED, - ), - default=DEFAULT_AUTOMATION_ENABLED, + ) ), celebration_enabled=_resolve_bool( _option_value( opts, CONF_CELEBRATION_ENABLED, DEFAULT_AUTOMATION_ENABLED - ), - default=DEFAULT_AUTOMATION_ENABLED, + ) ), ) + @classmethod + def from_options_resilient( + cls, options: Mapping[str, Any] | None = None + ) -> "NestQuestSettings": + """Build settings, falling back PER FIELD on a malformed value. -def _parse_hhmm_str(value: Any, *, field: str) -> str: - """Return a validated ``HH:MM`` string, raising on a malformed value. + Unlike :meth:`from_options` (which raises on the first invalid + field), this resolves EACH field in its own try/except: a + malformed value logs a :data:`~.const.LOGGER` warning naming the + field and falls back to that field's default. The integration + uses this at setup so a single bad sibling option (e.g. a + migrated ``morning_summary_time`` of ``'25:00'``) cannot + silently reset the user's ``horizon_days`` or + ``day_rollover_time`` — the two fields core actually consumes. - A separate helper from :func:`_parse_hhmm` because :meth:`from_options` - needs the original string kept (the dataclass stores strings, not - tuples), but still wants the same strict validation. - """ - if not isinstance(value, str) or _HHMM_PATTERN.fullmatch(value) is None: - raise ValueError( - f"{field} must be a strict HH:MM string, got {value!r}" + An absent or ``None`` key maps to the default via + :func:`_option_value` WITHOUT a warning (that is the normal + missing-key path); only a present, non-``None``, invalid value + triggers a warning and a per-field fallback. + """ + opts: Mapping[str, Any] = ( + MappingProxyType({}) if options is None else MappingProxyType(options) ) - return value + resolved: dict[str, Any] = {} + + def _resolve( + field: str, key: str, default: Any, fn: Callable[[Any], Any] + ) -> None: + raw = _option_value(opts, key, default) + try: + resolved[field] = fn(raw) + except ValueError as err: + LOGGER.warning( + "NestQuest option %s is invalid (%s); falling back " + "to the default %r", + field, + err, + default, + ) + resolved[field] = default + + _resolve( + "horizon_days", + CONF_HORIZON_DAYS, + DEFAULT_HORIZON_DAYS, + _resolve_horizon_days, + ) + _resolve( + "day_rollover_time", + CONF_DAY_ROLLOVER_TIME, + DEFAULT_DAY_ROLLOVER_TIME, + lambda v: _validate_hhmm(v, field="day_rollover_time"), + ) + _resolve( + "notify_target", + CONF_NOTIFY_TARGET, + "", + _resolve_optional_str, + ) + _resolve( + "morning_summary_time", + CONF_MORNING_SUMMARY_TIME, + DEFAULT_MORNING_SUMMARY_TIME, + lambda v: _validate_hhmm(v, field="morning_summary_time"), + ) + _resolve( + "afternoon_reminder_time", + CONF_AFTERNOON_REMINDER_TIME, + DEFAULT_AFTERNOON_REMINDER_TIME, + lambda v: _validate_hhmm(v, field="afternoon_reminder_time"), + ) + _resolve( + "end_of_day_report_time", + CONF_END_OF_DAY_REPORT_TIME, + DEFAULT_END_OF_DAY_REPORT_TIME, + lambda v: _validate_hhmm(v, field="end_of_day_report_time"), + ) + _resolve( + "morning_summary_enabled", + CONF_MORNING_SUMMARY_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ) + _resolve( + "afternoon_reminder_enabled", + CONF_AFTERNOON_REMINDER_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ) + _resolve( + "end_of_day_report_enabled", + CONF_END_OF_DAY_REPORT_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ) + _resolve( + "celebration_enabled", + CONF_CELEBRATION_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ) + return cls(**resolved) diff --git a/custom_components/nestquest/options_flow.py b/custom_components/nestquest/options_flow.py index bf5c59f..22e3ee2 100644 --- a/custom_components/nestquest/options_flow.py +++ b/custom_components/nestquest/options_flow.py @@ -1,7 +1,6 @@ """Options flow for NestQuest post-setup settings.""" from __future__ import annotations -import re from typing import Any import voluptuous as vol @@ -33,18 +32,17 @@ from .const import ( DEFAULT_UPDATE_INTERVAL, DOMAIN, MIN_UPDATE_INTERVAL, + TIME_PATTERN, ) from .admin_allowlist import list_admin_ids, set_admin_ids MIN_HORIZON_DAYS = 1 MIN_PANEL_IDLE_TIMEOUT = 30 -_TIME_PATTERN = re.compile(r"(?:[01]\d|2[0-3]):[0-5]\d") - def _parse_time(value: str) -> bool: """Return True when value is a valid 24-hour HH:MM string.""" - return _TIME_PATTERN.fullmatch(value) is not None + return TIME_PATTERN.fullmatch(value) is not None def _async_validate(user_input: dict[str, Any]) -> dict[str, str]: diff --git a/tests/test_core_settings.py b/tests/test_core_settings.py index 070cc1c..c60efe2 100644 --- a/tests/test_core_settings.py +++ b/tests/test_core_settings.py @@ -1,26 +1,31 @@ """Tests for core/settings.py: the explicit, HA-free settings object. -Covers three contracts from task 4dd8f870: +Covers the contracts from task 4dd8f870 plus the review fixes: - ``NestQuestSettings.from_options`` applies defaults when keys are - absent and rejects invalid horizon/rollover values. + absent and rejects invalid horizon/rollover values; ``from_options`` + is strict (raises), while ``from_options_resilient`` falls back PER + FIELD so a malformed sibling cannot reset horizon_days/rollover. - An AST scan asserts no module under ``custom_components/nestquest/core/`` imports ``homeassistant``, - accesses a ``hass.config`` / config-entry attribute, or reads - config-entry options. (Docstring mentions are allowed.) + names ``hass``/``config_entry``, reads a ``.config``/``.options``/ + ``.data`` attribute, or dynamically reads them via ``getattr``/ + ``hasattr``. (Docstring mentions are allowed.) A negative-control + test feeds the scanner each bypass shape and asserts it flags them. - A settings object constructed with NON-default ``horizon_days`` and ``day_rollover_time`` drives a materialization whose resulting window - matches the non-default horizon. + matches the non-default horizon — via BOTH the core ``materialize`` + path and the INTEGRATION path (``_run_horizon_materialization`` + + ``_register_day_rollover_listener``). """ from __future__ import annotations import ast import datetime -import inspect from dataclasses import FrozenInstanceError from pathlib import Path +from types import SimpleNamespace from typing import Any -from unittest.mock import MagicMock import pytest @@ -206,6 +211,97 @@ def test_from_options_strips_notify_target() -> None: assert s_empty.notify_target == "" +# --------------------------------------------------------------------------- +# from_options_resilient: per-field fallback (P2-1). +# --------------------------------------------------------------------------- + + +def test_from_options_resilient_none_yields_all_defaults() -> None: + """A None mapping yields the all-defaults settings (no warnings).""" + s = NestQuestSettings.from_options_resilient(None) + assert s.horizon_days == DEFAULT_HORIZON_DAYS + assert s.day_rollover_time == DEFAULT_DAY_ROLLOVER_TIME + + +def test_from_options_resilient_reads_valid_keys() -> None: + """Valid keys are read exactly as from_options would read them.""" + s = NestQuestSettings.from_options_resilient( + {CONF_HORIZON_DAYS: 3, CONF_DAY_ROLLOVER_TIME: "03:30"} + ) + assert s.horizon_days == 3 + assert s.day_rollover_time == "03:30" + + +def test_from_options_resilient_keeps_horizon_when_sibling_is_bad( + caplog: pytest.LogCaptureFixture, +) -> None: + """A malformed SIBLING field cannot reset horizon_days or day_rollover_time. + + The old all-or-nothing fallback reset EVERY field when from_options + raised for ANY field: ``{horizon_days: 3, morning_summary_time: + '25:00'}`` yielded horizon 14 (was 3). The resilient builder falls + back PER FIELD, so horizon_days=3 and day_rollover_time='03:30' + survive a malformed morning_summary_time, and the bad field alone + falls back to its default with a warning naming it. + """ + s = NestQuestSettings.from_options_resilient( + { + CONF_HORIZON_DAYS: 3, + CONF_DAY_ROLLOVER_TIME: "03:30", + CONF_MORNING_SUMMARY_TIME: "25:00", + } + ) + assert s.horizon_days == 3 + assert s.day_rollover_time == "03:30" + # The malformed sibling fell back to its default. + assert s.morning_summary_time == DEFAULT_MORNING_SUMMARY_TIME + # A warning was logged naming the bad field. + assert any( + "morning_summary_time" in record.getMessage() + and record.levelname == "WARNING" + for record in caplog.records + ) + + +def test_from_options_resilient_bad_horizon_falls_back_with_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + """A malformed horizon_days itself falls back to the default + warning.""" + s = NestQuestSettings.from_options_resilient({CONF_HORIZON_DAYS: True}) + assert s.horizon_days == DEFAULT_HORIZON_DAYS + assert any( + "horizon_days" in record.getMessage() and record.levelname == "WARNING" + for record in caplog.records + ) + + +def test_from_options_resilient_bad_rollover_falls_back_with_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + """A malformed day_rollover_time falls back to the default + warning.""" + s = NestQuestSettings.from_options_resilient( + {CONF_DAY_ROLLOVER_TIME: "99:99"} + ) + assert s.day_rollover_time == DEFAULT_DAY_ROLLOVER_TIME + assert any( + "day_rollover_time" in record.getMessage() + and record.levelname == "WARNING" + for record in caplog.records + ) + + +def test_from_options_resilient_none_value_is_not_a_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + """A None value maps to the default silently (it is absent, not malformed).""" + s = NestQuestSettings.from_options_resilient( + {CONF_HORIZON_DAYS: None, CONF_MORNING_SUMMARY_TIME: None} + ) + assert s.horizon_days == DEFAULT_HORIZON_DAYS + assert s.morning_summary_time == DEFAULT_MORNING_SUMMARY_TIME + assert not [r for r in caplog.records if r.levelname == "WARNING"] + + # --------------------------------------------------------------------------- # Construction + frozenness. # --------------------------------------------------------------------------- @@ -221,6 +317,42 @@ def test_direct_construction_validates() -> None: NestQuestSettings(morning_summary_enabled="yes") +@pytest.mark.parametrize( + "field,kwargs", + [ + ("horizon_days", {"horizon_days": None}), + ("day_rollover_time", {"day_rollover_time": None}), + ("notify_target", {"notify_target": None}), + ("morning_summary_time", {"morning_summary_time": None}), + ("afternoon_reminder_time", {"afternoon_reminder_time": None}), + ("end_of_day_report_time", {"end_of_day_report_time": None}), + ("morning_summary_enabled", {"morning_summary_enabled": None}), + ("afternoon_reminder_enabled", {"afternoon_reminder_enabled": None}), + ("end_of_day_report_enabled", {"end_of_day_report_enabled": None}), + ("celebration_enabled", {"celebration_enabled": None}), + ], +) +def test_direct_construction_rejects_none(field: str, kwargs: dict[str, Any]) -> None: + """__post_init__ is STRICT: None is rejected for every field. + + P2-5: the None->default mapping lives ONLY in from_options (via + _option_value). A hand-built NestQuestSettings(field=None) must raise + in __post_init__ rather than constructing and later blowing up + horizon_window (timedelta(days=None) -> TypeError) or the rollover + listener. + """ + with pytest.raises(ValueError): + NestQuestSettings(**kwargs) + + +@pytest.mark.parametrize("value", [True, False, "5", 1.5]) +def test_direct_construction_rejects_non_int_horizon(value: Any) -> None: + """A bool or non-int horizon is rejected at construction (not bool-as-int).""" + with pytest.raises(ValueError, match="horizon_days"): + NestQuestSettings(horizon_days=value) + + + def test_settings_is_frozen() -> None: """The dataclass is frozen so a stale settings object cannot be mutated.""" s = NestQuestSettings() @@ -240,17 +372,12 @@ def test_day_rollover_hour_minute() -> None: # --------------------------------------------------------------------------- -def test_horizon_window_default_today() -> None: - """horizon_window returns [today, today + horizon_days] when today is omitted.""" - s = NestQuestSettings(horizon_days=7) - start, end = s.horizon_window() - today = datetime.date.today() - assert start == today - assert end == today + datetime.timedelta(days=7) - - def test_horizon_window_pinned_today() -> None: - """A pinned today is honored so the window is deterministic.""" + """A pinned today is honored so the window is deterministic. + + ``today`` is REQUIRED (no host-clock default) — see + :meth:`NestQuestSettings.horizon_window`. + """ s = NestQuestSettings(horizon_days=5) pinned = datetime.date(2026, 3, 1) start, end = s.horizon_window(pinned) @@ -258,6 +385,13 @@ def test_horizon_window_pinned_today() -> None: assert end == pinned + datetime.timedelta(days=5) +def test_horizon_window_requires_today() -> None: + """horizon_window has no default today: omitting it raises TypeError.""" + s = NestQuestSettings(horizon_days=5) + with pytest.raises(TypeError): + s.horizon_window() # type: ignore[call-arg] + + def test_horizon_window_non_default_horizon() -> None: """A non-default horizon_days widens the window accordingly.""" pinned = datetime.date(2026, 3, 1) @@ -283,37 +417,57 @@ def _is_homeassistant(module: str) -> bool: return module == "homeassistant" or module.startswith("homeassistant.") -def _attribute_chain(node: ast.Attribute) -> list[str]: - """Flatten an attribute access chain into dotted-name parts (leaf first).""" - parts: list[str] = [node.attr] - cur: ast.expr = node.value - while isinstance(cur, ast.Attribute): - parts.append(cur.attr) - cur = cur.value - if isinstance(cur, ast.Name): - parts.append(cur.id) - return parts +#: HA-specific identifiers with no legitimate core use. ``entry`` is +#: deliberately NOT in this set: core uses ``entry`` as a domain loop +#: variable (iterating weekday_set entries in recurrence/presence/ +#: quest_definitions), so forbidding the bare name would false-positive. +#: The config-entry attribute reads the bare-name check misses are +#: caught by _CONFIG_ATTRS and _FORBIDDEN_ATTRS below. +_FORBIDDEN_NAMES = frozenset({"hass", "config_entry"}) + +#: Attribute names that signal a HA object reference (self.hass, +#: self.entry, obj.config_entry). No core module currently uses any of +#: these as an attribute, so flagging them is false-positive-free. +_FORBIDDEN_ATTRS = frozenset({"hass", "entry", "config_entry"}) + +#: Config-entry attribute reads — the shapes ``hass.config``, +#: ``config_entry.options``, ``self.entry.options``, ``runtime.options``, +#: ``entry.data``. Flagging the attribute ANYWHERE (regardless of root) +#: is what catches ``runtime.options``, which the old root-only check +#: missed. No core module currently reads any of these attributes. +_CONFIG_ATTRS = frozenset({"config", "options", "data"}) + +#: Dynamic config-entry reads via getattr/hasattr with a string argument. +_GETATTR_FUNCS = frozenset({"getattr", "hasattr"}) -def _dotted_attr(node: ast.Attribute) -> str: - """Return the dotted name of an attribute chain, root first.""" - return ".".join(reversed(_attribute_chain(node))) +def _scan_source_for_ha_config( + source: str, *, name: str = "" +) -> list[str]: + """Return offender strings for HA-coupling in ``source``. + Flags (each closes a bypass the old root-only ``hass``/``entry`` + chain check missed): -def _scan_module_for_ha_config(path: Path) -> list[str]: - """Return a list of offender strings for HA-coupling in ``path``. - - Flags: - any ``import homeassistant`` / ``from homeassistant ...``; - - any attribute access whose chain reaches ``hass.config`` or any - ``.options``/``.data`` access on a name literally named ``entry`` - (a config-entry attribute read), EXCEPT inside a docstring-only - module (no runtime access) — but since AST only sees code, not - docstrings, docstring mentions are structurally exempt. - - any call to ``.options.get(...)`` / ``.data.get(...)`` on a name - literally named ``entry`` (a config-entry options/data read). + - any ``Name``/``arg`` whose id is ``hass`` or ``config_entry`` + (HA-specific identifiers with no legitimate core use); + - any attribute access whose ``attr`` is ``hass``/``entry``/ + ``config_entry`` (catches ``self.hass``, ``self.entry``, + ``obj.config_entry`` — root names the old check could not see); + - any attribute access whose ``attr`` is ``config``/``options``/ + ``data`` (catches ``hass.config``, ``config_entry.options``, + ``self.entry.options``, ``runtime.options`` regardless of root); + - any ``getattr``/``hasattr`` call whose string argument is + ``'config'``/``'options'``/``'data'`` (catches the dynamic read + ``getattr(hass, 'config')`` the static attribute check cannot see). + + Docstring mentions are structurally exempt: AST only sees code, and + none of these node shapes appear inside a string literal. The + existing docstring references to ``hass.config`` in ``core/events.py`` + and ``core/store.py`` therefore pass. """ - tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + tree = ast.parse(source, filename=name) offenders: list[str] = [] for node in ast.walk(tree): @@ -321,51 +475,75 @@ def _scan_module_for_ha_config(path: Path) -> list[str]: for alias in node.names: if _is_homeassistant(alias.name): offenders.append( - f"{path.name}:{node.lineno} homeassistant import: " + f"{name}:{node.lineno} homeassistant import: " f"import {alias.name}" ) elif isinstance(node, ast.ImportFrom): base = node.module or "" if _is_homeassistant(base): offenders.append( - f"{path.name}:{node.lineno} homeassistant import: from {base}" + f"{name}:{node.lineno} homeassistant import: from {base}" ) for alias in node.names: - combined = ( - f"{base}.{alias.name}" if base else alias.name - ) + combined = f"{base}.{alias.name}" if base else alias.name if _is_homeassistant(combined): offenders.append( - f"{path.name}:{node.lineno} homeassistant import: " + f"{name}:{node.lineno} homeassistant import: " f"from {base} import {alias.name}" ) + elif isinstance(node, ast.Name): + if node.id in _FORBIDDEN_NAMES: + offenders.append( + f"{name}:{node.lineno} forbidden identifier: {node.id!r}" + ) + elif isinstance(node, ast.arg): + if node.arg in _FORBIDDEN_NAMES: + offenders.append( + f"{name}:{node.lineno} forbidden parameter: {node.arg!r}" + ) elif isinstance(node, ast.Attribute): - dotted = _dotted_attr(node) - # hass.config. — a HA config read. - if dotted == "hass.config" or dotted.startswith("hass.config."): + if node.attr in _FORBIDDEN_ATTRS: offenders.append( - f"{path.name}:{node.lineno} hass.config access: {dotted}" + f"{name}:{node.lineno} forbidden attribute: .{node.attr}" ) - # entry.options / entry.data — a config-entry attribute read. - # Only flag the leaf attribute itself (not every sub-expression), - # and only when the root name is literally 'entry'. - root = _attribute_chain(node)[-1] - if root == "entry" and node.attr in {"options", "data"}: + if node.attr in _CONFIG_ATTRS: offenders.append( - f"{path.name}:{node.lineno} config-entry attribute read: " - f"{dotted}" + f"{name}:{node.lineno} config-entry attribute read: " + f".{node.attr}" ) + elif isinstance(node, ast.Call): + func = node.func + if isinstance(func, ast.Name) and func.id in _GETATTR_FUNCS: + for arg in node.args: + if ( + isinstance(arg, ast.Constant) + and isinstance(arg.value, str) + and arg.value in _CONFIG_ATTRS + ): + offenders.append( + f"{name}:{node.lineno} {func.id}() dynamic " + f"config-entry read: {arg.value!r}" + ) return offenders +def _scan_module_for_ha_config(path: Path) -> list[str]: + """Scan a core module file for HA-coupling (file-backed scan).""" + return _scan_source_for_ha_config( + path.read_text(encoding="utf-8"), name=path.name + ) + + def test_core_modules_have_no_ha_config_reads() -> None: """No core module imports homeassistant or reads HA config / entry options. - Docstring mentions are allowed: AST only sees code, and the offenders - this scan flags (imports, attribute accesses) never appear in a - docstring. The existing docstring references to ``hass.config`` in - ``core/events.py`` and ``core/store.py`` therefore pass. + The scan forbids the HA-specific identifiers ``hass``/``config_entry``, + the HA-object attributes ``.hass``/``.entry``/``.config_entry``, the + config-entry attribute reads ``.config``/``.options``/``.data``, and + dynamic ``getattr``/``hasattr`` reads of them — so a chain rooted at + ``self`` or ``runtime`` (which the old root-only check missed) is now + caught. Docstring mentions pass: AST only sees code. """ py_files = sorted(CORE_PKG.rglob("*.py")) assert py_files, "core package not found" @@ -378,6 +556,49 @@ def test_core_modules_have_no_ha_config_reads() -> None: ) +def test_ast_scan_flags_each_bypass_shape() -> None: + """Negative control: the scanner flags each bypass shape the old check missed. + + The old scan only flagged chains rooted at a literal name ``hass``/ + ``entry``. Each snippet below is a shape it missed; the new scanner + must flag every one. + """ + snippets: dict[str, str] = { + "self.hass.config": "x = self.hass.config\n", + "config_entry.options": "x = config_entry.options\n", + "self.entry.options": "x = self.entry.options\n", + "getattr(hass,'config')": "x = getattr(hass, 'config')\n", + "runtime.options": "x = runtime.options\n", + "hasattr(obj,'data')": "x = hasattr(obj, 'data')\n", + } + for label, src in snippets.items(): + offenders = _scan_source_for_ha_config(src, name=label) + assert offenders, f"scanner missed {label!r}: {src!r}" + + +def test_ast_scan_allows_legitimate_entry_loop_var() -> None: + """A bare ``entry`` loop variable (a domain concept) is NOT flagged. + + ``entry`` is used in core as a loop variable over weekday_set entries + (recurrence.py, presence.py, quest_definitions.py) — not a HA config + entry. Forbidding the bare name would false-positive on those, so + only ``hass``/``config_entry`` are forbidden as bare names; the + config-entry reads via ``entry.options``/``entry.data`` are caught by + the ``_CONFIG_ATTRS`` attribute check instead. + """ + offenders = _scan_source_for_ha_config( + "for entry in items:\n pass\n", name="loop" + ) + assert offenders == [], offenders + + +def test_ast_scan_allows_clean_core_shape() -> None: + """A clean snippet with no HA coupling is not flagged (no false positives).""" + src = "opts = {'a': 1}\nx = opts.get('a')\nfor entry in items:\n pass\n" + offenders = _scan_source_for_ha_config(src, name="clean") + assert offenders == [], offenders + + def test_settings_module_lives_under_core() -> None: """settings.py is part of the core package (not the integration root).""" assert (CORE_PKG / "settings.py").is_file() @@ -385,56 +606,39 @@ def test_settings_module_lives_under_core() -> None: import custom_components.nestquest.core.settings as mod assert hasattr(mod, "NestQuestSettings") - assert inspect.isclass(mod.NestQuestSettings) + assert isinstance(mod.NestQuestSettings, type) def test_settings_module_has_no_ha_imports() -> None: - """settings.py specifically carries no homeassistant import.""" + """settings.py specifically carries no homeassistant import or config read.""" offenders = _scan_module_for_ha_config(CORE_PKG / "settings.py") assert offenders == [], offenders # --------------------------------------------------------------------------- -# Materialization over a non-default horizon. +# Materialization over a non-default horizon: core path + integration path. # --------------------------------------------------------------------------- -def _make_hass_mock(): - from unittest.mock import AsyncMock - - hass = MagicMock() - hass.async_add_executor_job = AsyncMock( - side_effect=(lambda fn, *a, **k: fn(*a, **k)) - ) - return hass - - -async def _prepare(path): +async def _prepare(path, executor): + """Open and migrate a database backed by ``executor`` (executor-only DB access).""" from custom_components.nestquest.core.db import NestQuestDatabase from custom_components.nestquest.core.migrations import apply_migrations - database = NestQuestDatabase(_make_hass_mock().async_add_executor_job) + database = NestQuestDatabase(executor) await database.open(path) await apply_migrations(database) return database -def _run(coro): - import asyncio - - return asyncio.new_event_loop().run_until_complete(coro) - - -def test_materialize_over_non_default_horizon(tmp_path) -> None: +async def test_materialize_over_non_default_horizon(hass, tmp_path) -> None: """A settings object with a non-default horizon drives the matching window. Constructs NestQuestSettings with horizon_days=3 and day_rollover_time '03:30' (both non-default), builds the window via horizon_window, and - drives materialize over it; asserts the resulting instance rows land - exactly on the [today, today+3] window and NOT beyond it. + drives the CORE ``materialize`` over it; asserts the resulting instance + rows land exactly on the [today, today+3] window and NOT beyond it. """ - import asyncio - from custom_components.nestquest.core.dao_children import ChildrenDao from custom_components.nestquest.core.dao_instances import ( QuestInstancesDao, @@ -448,57 +652,167 @@ def test_materialize_over_non_default_horizon(tmp_path) -> None: ScheduleRule, ) - async def _main(): - database = await _prepare(tmp_path / "nq.db") + database = await _prepare(tmp_path / "nq.db", hass.async_add_executor_job) + try: + settings = NestQuestSettings( + horizon_days=3, day_rollover_time="03:30" + ) + assert settings.horizon_days == 3 + assert settings.day_rollover_time == "03:30" + + today = datetime.date(2026, 3, 1) + start, end = settings.horizon_window(today) + assert start == today + assert end == today + datetime.timedelta(days=3) + + NOW = "2026-03-01T12:00:00+00:00" + child = await ChildrenDao(database).create("Ada", NOW) + await create_quest_definition( + database, + "Daily chore", + ScheduleRule( + rule_type=RuleType.DAILY, start_date=today.isoformat() + ), + [child.id], + ["morning"], + ) + + count = await materialize( + database, + start.isoformat(), + end.isoformat(), + today=today, + ) + # 4 days (today..today+3 inclusive), one instance per day. + assert count == 4 + + records = await QuestInstancesDao(database).list_by_date_range( + child.id, start.isoformat(), end.isoformat() + ) + dates = sorted({r.due_date for r in records}) + assert dates == [ + (today + datetime.timedelta(days=i)).isoformat() + for i in range(4) + ] + + # Beyond the configured horizon: no instances. + beyond = (end + datetime.timedelta(days=1)).isoformat() + far = (end + datetime.timedelta(days=10)).isoformat() + assert await QuestInstancesDao(database).list_by_date_range( + child.id, beyond, far + ) == [] + finally: + await database.close() + + +def _freeze_nestquest_clock( + monkeypatch, instant: datetime.datetime +) -> None: + """Freeze the integration's ``datetime`` clock to a fixed aware instant. + + Only ``custom_components.nestquest``'s ``datetime`` binding is patched, + so ``_run_horizon_materialization``'s ``datetime.datetime.now`` reads + this instant (localized to ``hass.config.time_zone``) while the test + harness and core keep the real clock. Mirrors the helper in + test_timezones.py. + """ + import custom_components.nestquest as nestquest + + frozen = instant.astimezone(datetime.timezone.utc) + + class _FrozenDatetime(datetime.datetime): + @classmethod + def now(cls, tz=None): + if tz is None: + return frozen.replace(tzinfo=None) + return frozen.astimezone(tz) + + fake_module = SimpleNamespace( + datetime=_FrozenDatetime, + timedelta=datetime.timedelta, + date=datetime.date, + ) + monkeypatch.setattr(nestquest, "datetime", fake_module) + + +async def test_integration_path_threads_settings( + hass, tmp_path, monkeypatch +) -> None: + """The integration path threads ``settings`` through horizon_window + rollover. + + P2-3: ``_run_horizon_materialization`` now calls + ``settings.horizon_window(today)`` (so the helper is real and shared, + not recomputed inline), and ``_register_day_rollover_listener`` + registers at ``settings.day_rollover_hour_minute``. This runs the + INTEGRATION path with ``horizon_days=3`` and + ``day_rollover_time='03:30'`` and asserts the resulting instance + window matches [today, today+3] AND the registered hour/minute are 3/30 + — a test the requirement-level core-only check could not fail. + """ + import custom_components.nestquest as nestquest + from custom_components.nestquest.core.dao_children import ChildrenDao + from custom_components.nestquest.core.dao_instances import ( + QuestInstancesDao, + ) + from custom_components.nestquest.core.quest_definitions import ( + create_quest_definition, + ) + from custom_components.nestquest.core.recurrence import ( + RuleType, + ScheduleRule, + ) + + # Freeze the integration clock so "today" is deterministic (the + # hass fixture's config.time_zone is UTC, so 2026-03-01 12:00 UTC + # localizes to 2026-03-01). + _freeze_nestquest_clock( + monkeypatch, datetime.datetime(2026, 3, 1, 12, 0, tzinfo=datetime.timezone.utc) + ) + + database = await _prepare(tmp_path / "nq.db", hass.async_add_executor_job) + try: + settings = NestQuestSettings(horizon_days=3, day_rollover_time="03:30") + today = datetime.date(2026, 3, 1) + + NOW = "2026-03-01T12:00:00+00:00" + child = await ChildrenDao(database).create("Ada", NOW) + await create_quest_definition( + database, + "Daily chore", + ScheduleRule( + rule_type=RuleType.DAILY, start_date=today.isoformat() + ), + [child.id], + ["morning"], + ) + + # Integration path: the window comes from settings.horizon_window. + await nestquest._run_horizon_materialization(hass, database, settings) + + end = today + datetime.timedelta(days=3) + records = await QuestInstancesDao(database).list_by_date_range( + child.id, today.isoformat(), end.isoformat() + ) + dates = sorted({r.due_date for r in records}) + assert dates == [ + (today + datetime.timedelta(days=i)).isoformat() + for i in range(4) + ] + beyond = (end + datetime.timedelta(days=1)).isoformat() + far = (end + datetime.timedelta(days=10)).isoformat() + assert await QuestInstancesDao(database).list_by_date_range( + child.id, beyond, far + ) == [] + + # The rollover listener registers at the configured hour/minute. + remove = nestquest._register_day_rollover_listener( + hass, database, settings + ) try: - settings = NestQuestSettings( - horizon_days=3, day_rollover_time="03:30" - ) - assert settings.horizon_days == 3 - assert settings.day_rollover_time == "03:30" - - today = datetime.date(2026, 3, 1) - start, end = settings.horizon_window(today) - assert start == today - assert end == today + datetime.timedelta(days=3) - - NOW = "2026-03-01T12:00:00+00:00" - child = await ChildrenDao(database).create("Ada", NOW) - await create_quest_definition( - database, - "Daily chore", - ScheduleRule( - rule_type=RuleType.DAILY, start_date=today.isoformat() - ), - [child.id], - ["morning"], - ) - - count = await materialize( - database, - start.isoformat(), - end.isoformat(), - today=today, - ) - # 4 days (today..today+3 inclusive), one instance per day. - assert count == 4 - - records = await QuestInstancesDao(database).list_by_date_range( - child.id, start.isoformat(), end.isoformat() - ) - dates = sorted({r.due_date for r in records}) - assert dates == [ - (today + datetime.timedelta(days=i)).isoformat() - for i in range(4) - ] - - # Beyond the configured horizon: no instances. - beyond = (end + datetime.timedelta(days=1)).isoformat() - far = (end + datetime.timedelta(days=10)).isoformat() - assert await QuestInstancesDao(database).list_by_date_range( - child.id, beyond, far - ) == [] + reg = hass.time_change.registrations[-1] + assert reg["hour"] == 3 + assert reg["minute"] == 30 finally: - await database.close() - - asyncio.new_event_loop().run_until_complete(_main()) + remove() + finally: + await database.close() diff --git a/tests/test_lifecycle.py b/tests/test_lifecycle.py index 179a4b9..cb793f0 100644 --- a/tests/test_lifecycle.py +++ b/tests/test_lifecycle.py @@ -526,24 +526,29 @@ async def test_setup_backfills_configured_horizon_days(hass, make_entry) -> None ).list_by_date_range(child_id, beyond, beyond_end) == [] -def test_configured_horizon_days_validates_and_falls_back() -> None: - """The configured horizon is validated; a malformed value uses the default.""" - import custom_components.nestquest as nestquest +def test_from_options_horizon_validates_and_defaults() -> None: + """from_options returns the configured horizon, defaulting when absent/None. - assert nestquest._configured_horizon_days({CONF_HORIZON_DAYS: 5}) == 5 - assert nestquest._configured_horizon_days({}) == DEFAULT_HORIZON_DAYS + Repointed from the removed ``_configured_horizon_days`` shim: a valid + horizon is read, an absent or None key falls back to the default, and + a malformed value RAISES (the integration's ``from_options_resilient`` + handles the per-field fall-back at setup, not this strict path). + """ + from custom_components.nestquest.settings import NestQuestSettings + + assert NestQuestSettings.from_options({CONF_HORIZON_DAYS: 5}).horizon_days == 5 assert ( - nestquest._configured_horizon_days({CONF_HORIZON_DAYS: True}) - == DEFAULT_HORIZON_DAYS + NestQuestSettings.from_options({}).horizon_days == DEFAULT_HORIZON_DAYS ) assert ( - nestquest._configured_horizon_days({CONF_HORIZON_DAYS: "5"}) - == DEFAULT_HORIZON_DAYS - ) - assert ( - nestquest._configured_horizon_days({CONF_HORIZON_DAYS: 0}) + NestQuestSettings.from_options( + {CONF_HORIZON_DAYS: None} + ).horizon_days == DEFAULT_HORIZON_DAYS ) + for bad in (True, "5", 0): + with pytest.raises(ValueError): + NestQuestSettings.from_options({CONF_HORIZON_DAYS: bad}) async def test_regenerate_service_registered_and_materializes( -- 2.55.0 From d75ddc19bd69e0f77c9ea2bf888d59eb920229ef Mon Sep 17 00:00:00 2001 From: Joshua Date: Mon, 21 Sep 2026 15:42:26 -0400 Subject: [PATCH 3/4] Apply PR #144 review round 2: settings-based service kwargs, required settings, shim removal, hardened AST scan P2: services._generation_kwargs now reads runtime.settings.horizon_days instead of re-implementing the bool/non-int/<1 fallback against runtime.options; drop the now-unused CONF_HORIZON_DAYS/DEFAULT_HORIZON_DAYS imports. Remove NestQuestRuntimeData.options (kept only for that read; the admin-id path reads entry.options directly) and the matching test assertion. P3: _run_horizon_materialization's settings param is now REQUIRED (drop the '| None = None' default + the NestQuestSettings() substitution branch); update the test_timezones callers to pass NestQuestSettings() explicitly. P3: __post_init__ normalises notify_target via _resolve_optional_str (strip) so a hand-built NestQuestSettings(notify_target=' x ') behaves like the options flow; keep the docstring accurate. P3: delete the custom_components/nestquest/settings.py shim (settings is a new module with no legacy import path) and import NestQuestSettings from .core.settings directly in __init__.py, in alphabetical position. P3: harden the AST scan for importlib.import_module('homeassistant...') (string-based import) and call keyword names like hass= (ast.keyword); add negative-control cases for each; explain in the failure message why .config/.options/.data are banned (HA config-entry attributes with no legitimate core use). P3: drive from_options and from_options_resilient from one _FIELDS table of (field, key, default, validator) to avoid three-way drift. uv run pytest -q: 1573 passed. --- custom_components/nestquest/__init__.py | 22 +- custom_components/nestquest/core/settings.py | 220 ++++++++----------- custom_components/nestquest/services.py | 9 +- custom_components/nestquest/settings.py | 13 -- tests/test_core_settings.py | 110 +++++++++- tests/test_lifecycle.py | 6 +- tests/test_timezones.py | 17 +- 7 files changed, 221 insertions(+), 176 deletions(-) delete mode 100644 custom_components/nestquest/settings.py diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 5bfea62..7d01021 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -24,7 +24,7 @@ from .const import ( LOGGER, PLATFORMS, ) -from .settings import NestQuestSettings +from .core.settings import NestQuestSettings from .frontend import async_register_frontend from .coordinator import NestQuestCoordinator from .db import NestQuestDatabase, make_database @@ -237,7 +237,6 @@ class NestQuestRuntimeData: """Runtime data stored on a NestQuest config entry.""" entry_id: str - options: dict[str, Any] settings: NestQuestSettings database: NestQuestDatabase remove_update_listener: Callable[[], Any] @@ -270,7 +269,7 @@ async def _async_owner_user_ids(hass: HomeAssistant) -> list[str]: async def _run_horizon_materialization( hass: HomeAssistant, database: NestQuestDatabase, - settings: NestQuestSettings | None = None, + settings: NestQuestSettings, ) -> None: """Materialize the rolling horizon ``[today, today + horizon_days]``. @@ -281,14 +280,12 @@ async def _run_horizon_materialization( This is the ONE generation path: the startup backfill, the daily rollover listener and the ``regenerate`` service all funnel through it, so there is - no duplicated materialization logic. ``settings`` carries the entry's - validated ``horizon_days``; when omitted a default - :class:`NestQuestSettings` is used (its horizon is the configured - default). The window itself comes from :meth:`settings.horizon_window` - so the helper is real and shared (not recomputed inline here). + no duplicated materialization logic. ``settings`` (REQUIRED — there is no + silent default; every caller already passes the entry's validated + :class:`NestQuestSettings`) carries the configured ``horizon_days``. The + window itself comes from :meth:`settings.horizon_window` so the helper is + real and shared (not recomputed inline here). """ - if settings is None: - settings = NestQuestSettings() time_zone = ZoneInfo(hass.config.time_zone) today = datetime.datetime.now(time_zone).date() start, end = settings.horizon_window(today) @@ -329,7 +326,9 @@ def _find_live_runtime_data(hass: HomeAssistant) -> NestQuestRuntimeData | None: their own; they resolve whichever config entry's runtime data is currently live at call time, so none of them can hold a stale handle to a closed connection after another entry unloads. The record also - carries that entry's options (e.g. the configured horizon). + carries that entry's validated ``settings`` (e.g. the configured + horizon) so service handlers read horizon days off it rather than + re-reading HA config. """ for runtime_data in hass.data.get(DOMAIN, {}).values(): database = getattr(runtime_data, "database", None) @@ -505,7 +504,6 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: await coordinator.async_config_entry_first_refresh() runtime_data = NestQuestRuntimeData( entry_id=entry.entry_id, - options=dict(entry.options), settings=settings, database=database, remove_update_listener=remove_update_listener, diff --git a/custom_components/nestquest/core/settings.py b/custom_components/nestquest/core/settings.py index 25d5e57..c432335 100644 --- a/custom_components/nestquest/core/settings.py +++ b/custom_components/nestquest/core/settings.py @@ -142,13 +142,81 @@ def _resolve_optional_str(value: Any) -> str: padded value (e.g. one migrated from an older schema) by normalizing it rather than crashing setup. The ``None``-as-absent mapping lives ONLY in :meth:`NestQuestSettings.from_options` (via - :func:`_option_value`). + :func:`_option_value`). :meth:`NestQuestSettings.__post_init__` + applies this SAME normalizer so a hand-built + ``NestQuestSettings(notify_target=' x ')`` is stripped to ``'x'`` + exactly like the options-flow path (no padded value can slip through + direct construction either). """ if not isinstance(value, str): raise ValueError(f"notify_target must be a string, got {value!r}") return value.strip() +#: The single field table the two builders share. Each row is +#: ``(field_name, CONF_* key, default, validator)``. ``from_options`` +#: applies every validator (raising on the first invalid value) while +#: ``from_options_resilient`` wraps each in its own try/except so a single +#: bad sibling falls back to its default alone. Driving both builders +#: from this table avoids three-way drift between ``__post_init__``'s +#: inline checks and the two option builders' field lists: the only +#: places that enumerate the fields are this table and +#: ``__post_init__`` (which validates a hand-built instance without the +#: None->default mapping, so it stays inline). +_FIELDS: tuple[tuple[str, str, Any, Callable[[Any], Any]], ...] = ( + ("horizon_days", CONF_HORIZON_DAYS, DEFAULT_HORIZON_DAYS, _resolve_horizon_days), + ( + "day_rollover_time", + CONF_DAY_ROLLOVER_TIME, + DEFAULT_DAY_ROLLOVER_TIME, + lambda v: _validate_hhmm(v, field="day_rollover_time"), + ), + ("notify_target", CONF_NOTIFY_TARGET, "", _resolve_optional_str), + ( + "morning_summary_time", + CONF_MORNING_SUMMARY_TIME, + DEFAULT_MORNING_SUMMARY_TIME, + lambda v: _validate_hhmm(v, field="morning_summary_time"), + ), + ( + "afternoon_reminder_time", + CONF_AFTERNOON_REMINDER_TIME, + DEFAULT_AFTERNOON_REMINDER_TIME, + lambda v: _validate_hhmm(v, field="afternoon_reminder_time"), + ), + ( + "end_of_day_report_time", + CONF_END_OF_DAY_REPORT_TIME, + DEFAULT_END_OF_DAY_REPORT_TIME, + lambda v: _validate_hhmm(v, field="end_of_day_report_time"), + ), + ( + "morning_summary_enabled", + CONF_MORNING_SUMMARY_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ), + ( + "afternoon_reminder_enabled", + CONF_AFTERNOON_REMINDER_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ), + ( + "end_of_day_report_enabled", + CONF_END_OF_DAY_REPORT_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ), + ( + "celebration_enabled", + CONF_CELEBRATION_ENABLED, + DEFAULT_AUTOMATION_ENABLED, + _resolve_bool, + ), +) + + @dataclass(frozen=True) class NestQuestSettings: """Explicit, HA-free configuration carried into core paths. @@ -181,14 +249,20 @@ class NestQuestSettings: # cannot carry an invalid value: the same rules from_options # applies, but WITHOUT the None->default mapping (that lives # only in from_options via _option_value). A None horizon, time, - # toggle, or notify target raises here rather than later blowing - # up horizon_window or the rollover listener. + # or toggle raises here rather than later blowing up + # horizon_window or the rollover listener. _resolve_horizon_days(self.horizon_days) _validate_hhmm(self.day_rollover_time, field="day_rollover_time") - if not isinstance(self.notify_target, str): - raise ValueError( - f"notify_target must be a string, got {self.notify_target!r}" - ) + # ``notify_target`` is normalised the SAME way as from_options + # (via _resolve_optional_str): a non-str is rejected and a + # padded/whitespace-only value is STRIPPED to its trim. This + # keeps a hand-built ``NestQuestSettings(notify_target=' x ')`` + # behaving like the options flow (which strips on persist) so a + # padded value cannot slip through direct construction either. + # The dataclass is frozen, so the stripped value is written via + # object.__setattr__. + stripped_notify_target = _resolve_optional_str(self.notify_target) + object.__setattr__(self, "notify_target", stripped_notify_target) _validate_hhmm(self.morning_summary_time, field="morning_summary_time") _validate_hhmm( self.afternoon_reminder_time, field="afternoon_reminder_time" @@ -259,66 +333,10 @@ class NestQuestSettings: MappingProxyType({}) if options is None else MappingProxyType(options) ) return cls( - horizon_days=_resolve_horizon_days( - _option_value(opts, CONF_HORIZON_DAYS, DEFAULT_HORIZON_DAYS) - ), - day_rollover_time=_validate_hhmm( - _option_value( - opts, CONF_DAY_ROLLOVER_TIME, DEFAULT_DAY_ROLLOVER_TIME - ), - field="day_rollover_time", - ), - notify_target=_resolve_optional_str( - _option_value(opts, CONF_NOTIFY_TARGET, "") - ), - morning_summary_time=_validate_hhmm( - _option_value( - opts, - CONF_MORNING_SUMMARY_TIME, - DEFAULT_MORNING_SUMMARY_TIME, - ), - field="morning_summary_time", - ), - afternoon_reminder_time=_validate_hhmm( - _option_value( - opts, - CONF_AFTERNOON_REMINDER_TIME, - DEFAULT_AFTERNOON_REMINDER_TIME, - ), - field="afternoon_reminder_time", - ), - end_of_day_report_time=_validate_hhmm( - _option_value( - opts, - CONF_END_OF_DAY_REPORT_TIME, - DEFAULT_END_OF_DAY_REPORT_TIME, - ), - field="end_of_day_report_time", - ), - morning_summary_enabled=_resolve_bool( - _option_value( - opts, CONF_MORNING_SUMMARY_ENABLED, DEFAULT_AUTOMATION_ENABLED - ) - ), - afternoon_reminder_enabled=_resolve_bool( - _option_value( - opts, - CONF_AFTERNOON_REMINDER_ENABLED, - DEFAULT_AUTOMATION_ENABLED, - ) - ), - end_of_day_report_enabled=_resolve_bool( - _option_value( - opts, - CONF_END_OF_DAY_REPORT_ENABLED, - DEFAULT_AUTOMATION_ENABLED, - ) - ), - celebration_enabled=_resolve_bool( - _option_value( - opts, CONF_CELEBRATION_ENABLED, DEFAULT_AUTOMATION_ENABLED - ) - ), + **{ + field: validator(_option_value(opts, key, default)) + for field, key, default, validator in _FIELDS + } ) @classmethod @@ -345,13 +363,10 @@ class NestQuestSettings: MappingProxyType({}) if options is None else MappingProxyType(options) ) resolved: dict[str, Any] = {} - - def _resolve( - field: str, key: str, default: Any, fn: Callable[[Any], Any] - ) -> None: + for field, key, default, validator in _FIELDS: raw = _option_value(opts, key, default) try: - resolved[field] = fn(raw) + resolved[field] = validator(raw) except ValueError as err: LOGGER.warning( "NestQuest option %s is invalid (%s); falling back " @@ -361,65 +376,4 @@ class NestQuestSettings: default, ) resolved[field] = default - - _resolve( - "horizon_days", - CONF_HORIZON_DAYS, - DEFAULT_HORIZON_DAYS, - _resolve_horizon_days, - ) - _resolve( - "day_rollover_time", - CONF_DAY_ROLLOVER_TIME, - DEFAULT_DAY_ROLLOVER_TIME, - lambda v: _validate_hhmm(v, field="day_rollover_time"), - ) - _resolve( - "notify_target", - CONF_NOTIFY_TARGET, - "", - _resolve_optional_str, - ) - _resolve( - "morning_summary_time", - CONF_MORNING_SUMMARY_TIME, - DEFAULT_MORNING_SUMMARY_TIME, - lambda v: _validate_hhmm(v, field="morning_summary_time"), - ) - _resolve( - "afternoon_reminder_time", - CONF_AFTERNOON_REMINDER_TIME, - DEFAULT_AFTERNOON_REMINDER_TIME, - lambda v: _validate_hhmm(v, field="afternoon_reminder_time"), - ) - _resolve( - "end_of_day_report_time", - CONF_END_OF_DAY_REPORT_TIME, - DEFAULT_END_OF_DAY_REPORT_TIME, - lambda v: _validate_hhmm(v, field="end_of_day_report_time"), - ) - _resolve( - "morning_summary_enabled", - CONF_MORNING_SUMMARY_ENABLED, - DEFAULT_AUTOMATION_ENABLED, - _resolve_bool, - ) - _resolve( - "afternoon_reminder_enabled", - CONF_AFTERNOON_REMINDER_ENABLED, - DEFAULT_AUTOMATION_ENABLED, - _resolve_bool, - ) - _resolve( - "end_of_day_report_enabled", - CONF_END_OF_DAY_REPORT_ENABLED, - DEFAULT_AUTOMATION_ENABLED, - _resolve_bool, - ) - _resolve( - "celebration_enabled", - CONF_CELEBRATION_ENABLED, - DEFAULT_AUTOMATION_ENABLED, - _resolve_bool, - ) return cls(**resolved) diff --git a/custom_components/nestquest/services.py b/custom_components/nestquest/services.py index 0c66635..2ce49dd 100644 --- a/custom_components/nestquest/services.py +++ b/custom_components/nestquest/services.py @@ -24,8 +24,6 @@ from . import children as children_layer from . import completion as completion_layer from . import quest_definitions as quest_definitions_layer from .const import ( - CONF_HORIZON_DAYS, - DEFAULT_HORIZON_DAYS, DOMAIN, DOMAIN_SERVICES, QUEST_WINDOWS, @@ -258,12 +256,7 @@ async def _refresh_entities(runtime: Any) -> None: def _generation_kwargs(hass: HomeAssistant, runtime: Any) -> dict[str, Any]: time_zone = ZoneInfo(hass.config.time_zone) today = datetime.datetime.now(time_zone).date() - value = runtime.options.get(CONF_HORIZON_DAYS, DEFAULT_HORIZON_DAYS) - if isinstance(value, bool) or not isinstance(value, int) or value < 1: - horizon_days = DEFAULT_HORIZON_DAYS - else: - horizon_days = value - return {"today": today, "horizon_days": horizon_days} + return {"today": today, "horizon_days": runtime.settings.horizon_days} def _actor_kwargs(call: Any) -> dict[str, Any]: diff --git a/custom_components/nestquest/settings.py b/custom_components/nestquest/settings.py deleted file mode 100644 index 9e8f62a..0000000 --- a/custom_components/nestquest/settings.py +++ /dev/null @@ -1,13 +0,0 @@ -"""Re-export of :mod:`custom_components.nestquest.core.settings` for the integration package. - -The explicit, Home-Assistant-free settings object lives in -:mod:`custom_components.nestquest.core.settings` (introduced in task -4dd8f870); this module re-exports it so the integration constructs the -settings object from ``entry.options`` and threads it into the core -paths without core ever reading HA config itself. -""" -from __future__ import annotations - -from .core.settings import NestQuestSettings - -__all__ = ["NestQuestSettings"] diff --git a/tests/test_core_settings.py b/tests/test_core_settings.py index c60efe2..c07f456 100644 --- a/tests/test_core_settings.py +++ b/tests/test_core_settings.py @@ -353,6 +353,19 @@ def test_direct_construction_rejects_non_int_horizon(value: Any) -> None: +def test_direct_construction_strips_notify_target() -> None: + """__post_init__ normalises notify_target the SAME way from_options does. + + P3: a hand-built ``NestQuestSettings(notify_target=' x ')`` is + stripped to ``'x'`` (and a whitespace-only value to ``''``) so direct + construction behaves like the options flow; a padded value cannot + slip through __post_init__ either. + """ + assert NestQuestSettings(notify_target=" notify.x ").notify_target == "notify.x" + assert NestQuestSettings(notify_target=" ").notify_target == "" + assert NestQuestSettings(notify_target="notify.x").notify_target == "notify.x" + + def test_settings_is_frozen() -> None: """The dataclass is frozen so a stale settings object cannot be mutated.""" s = NestQuestSettings() @@ -440,6 +453,22 @@ _CONFIG_ATTRS = frozenset({"config", "options", "data"}) #: Dynamic config-entry reads via getattr/hasattr with a string argument. _GETATTR_FUNCS = frozenset({"getattr", "hasattr"}) +#: The dynamic-import function the string-based import bypass uses. +#: ``importlib.import_module("homeassistant...")`` reaches the HA package +#: without an ``import homeassistant`` statement the static import check +#: sees, so the scanner must recognise the call shape too. +_IMPORT_MODULE_FUNCS = frozenset({"import_module"}) + + +def _is_import_module_call(node: ast.Call) -> bool: + """True for ``importlib.import_module(...)`` or a bare ``import_module(...)``.""" + func = node.func + if isinstance(func, ast.Attribute): + return func.attr in _IMPORT_MODULE_FUNCS + if isinstance(func, ast.Name): + return func.id in _IMPORT_MODULE_FUNCS + return False + def _scan_source_for_ha_config( source: str, *, name: str = "" @@ -450,14 +479,24 @@ def _scan_source_for_ha_config( chain check missed): - any ``import homeassistant`` / ``from homeassistant ...``; + - any ``importlib.import_module('homeassistant...')`` (a string-based + import that reaches HA without an ``import homeassistant`` statement + the static import check can see); - any ``Name``/``arg`` whose id is ``hass`` or ``config_entry`` (HA-specific identifiers with no legitimate core use); + - any call keyword whose name is ``hass`` or ``config_entry`` + (catches ``foo(hass=...)`` / ``handler(config_entry=...)`` — the + bare-Name check misses keyword arguments); - any attribute access whose ``attr`` is ``hass``/``entry``/ ``config_entry`` (catches ``self.hass``, ``self.entry``, ``obj.config_entry`` — root names the old check could not see); - any attribute access whose ``attr`` is ``config``/``options``/ - ``data`` (catches ``hass.config``, ``config_entry.options``, - ``self.entry.options``, ``runtime.options`` regardless of root); + ``data`` — these are Home Assistant config-entry attributes with no + legitimate core use (``.config`` reads ``hass.config``, ``.options`` + reads ``config_entry.options``, ``.data`` reads ``config_entry.data``); + flagging the attribute ANYWHERE (regardless of root) is what catches + ``runtime.options``, which the old root-only check missed. No core + module currently reads any of these attributes; - any ``getattr``/``hasattr`` call whose string argument is ``'config'``/``'options'``/``'data'`` (catches the dynamic read ``getattr(hass, 'config')`` the static attribute check cannot see). @@ -501,6 +540,16 @@ def _scan_source_for_ha_config( offenders.append( f"{name}:{node.lineno} forbidden parameter: {node.arg!r}" ) + elif isinstance(node, ast.keyword): + # A call keyword like ``foo(hass=...)``: the bare-Name check + # sees the value, not the keyword's own name, so a HA object + # could be passed by keyword without tripping it. ``arg`` is + # ``None`` for ``**kwargs`` unpacking, which is not a name. + if node.arg is not None and node.arg in _FORBIDDEN_NAMES: + offenders.append( + f"{name}:{node.lineno} forbidden keyword argument: " + f"{node.arg!r}" + ) elif isinstance(node, ast.Attribute): if node.attr in _FORBIDDEN_ATTRS: offenders.append( @@ -512,6 +561,19 @@ def _scan_source_for_ha_config( f".{node.attr}" ) elif isinstance(node, ast.Call): + if _is_import_module_call(node): + # The first positional argument is the module string; a + # constant ``'homeassistant...'`` is the bypass shape. + for arg in node.args: + if ( + isinstance(arg, ast.Constant) + and isinstance(arg.value, str) + and _is_homeassistant(arg.value) + ): + offenders.append( + f"{name}:{node.lineno} dynamic homeassistant " + f"import: import_module({arg.value!r})" + ) func = node.func if isinstance(func, ast.Name) and func.id in _GETATTR_FUNCS: for arg in node.args: @@ -543,7 +605,13 @@ def test_core_modules_have_no_ha_config_reads() -> None: config-entry attribute reads ``.config``/``.options``/``.data``, and dynamic ``getattr``/``hasattr`` reads of them — so a chain rooted at ``self`` or ``runtime`` (which the old root-only check missed) is now - caught. Docstring mentions pass: AST only sees code. + caught. ``.config``/``.options``/``.data`` are banned because they are + Home Assistant config-entry attributes with no legitimate core use: + core receives the resolved values via the explicit + :class:`NestQuestSettings` object the integration builds from + ``entry.options``, so any read of those attributes in ``core/`` is a + config-entry coupling the settings object exists to remove. + Docstring mentions pass: AST only sees code. """ py_files = sorted(CORE_PKG.rglob("*.py")) assert py_files, "core package not found" @@ -570,12 +638,48 @@ def test_ast_scan_flags_each_bypass_shape() -> None: "getattr(hass,'config')": "x = getattr(hass, 'config')\n", "runtime.options": "x = runtime.options\n", "hasattr(obj,'data')": "x = hasattr(obj, 'data')\n", + "importlib.import_module('homeassistant')": ( + "import importlib\nimportlib.import_module('homeassistant')\n" + ), + "import_module('homeassistant.helpers')": ( + "from importlib import import_module\n" + "import_module('homeassistant.helpers')\n" + ), + "handler(hass=...)": "handler(hass=obj)\n", + "handler(config_entry=...)": "handler(config_entry=obj)\n", } for label, src in snippets.items(): offenders = _scan_source_for_ha_config(src, name=label) assert offenders, f"scanner missed {label!r}: {src!r}" +def test_ast_scan_allows_non_homeassistant_import_module() -> None: + """A dynamic import of a NON-homeassistant module is not flagged. + + ``importlib.import_module`` is a legitimate way to load a non-HA + module lazily; only a ``homeassistant...`` target is the bypass shape. + """ + src = ( + "import importlib\n" + "m = importlib.import_module('custom_components.nestquest.core')\n" + ) + assert _scan_source_for_ha_config(src, name="non-ha-import") == [] + + +def test_ast_scan_allows_non_forbidden_keyword_arguments() -> None: + """A call with non-forbidden keyword arguments is not flagged. + + Only ``hass=``/``config_entry=`` are forbidden as keyword names; an + unrelated keyword like ``hass_data=`` or ``entry_id=`` must not + false-positive (and ``**kwargs`` unpacking has ``arg=None``). + """ + src = ( + "foo(entry_id=1, name='x')\n" + "bar(**kwargs)\n" + ) + assert _scan_source_for_ha_config(src, name="non-forbidden-kwargs") == [] + + def test_ast_scan_allows_legitimate_entry_loop_var() -> None: """A bare ``entry`` loop variable (a domain concept) is NOT flagged. diff --git a/tests/test_lifecycle.py b/tests/test_lifecycle.py index cb793f0..8955c3f 100644 --- a/tests/test_lifecycle.py +++ b/tests/test_lifecycle.py @@ -36,12 +36,12 @@ async def test_setup_stores_runtime_data_in_hass_data(hass, make_entry) -> None: assert hass.data[DOMAIN][entry.entry_id] is entry.runtime_data -async def test_setup_runtime_data_has_entry_id_and_options(hass, make_entry) -> None: +async def test_setup_runtime_data_has_entry_id_and_settings(hass, make_entry) -> None: entry = _wire(make_entry(options={"horizon_days": 7}), hass.registry) await async_setup_entry(hass, entry) runtime = entry.runtime_data assert runtime.entry_id == entry.entry_id - assert runtime.options == {"horizon_days": 7} + assert runtime.settings.horizon_days == 7 assert callable(runtime.remove_update_listener) @@ -534,7 +534,7 @@ def test_from_options_horizon_validates_and_defaults() -> None: a malformed value RAISES (the integration's ``from_options_resilient`` handles the per-field fall-back at setup, not this strict path). """ - from custom_components.nestquest.settings import NestQuestSettings + from custom_components.nestquest.core.settings import NestQuestSettings assert NestQuestSettings.from_options({CONF_HORIZON_DAYS: 5}).horizon_days == 5 assert ( diff --git a/tests/test_timezones.py b/tests/test_timezones.py index e775429..09d8271 100644 --- a/tests/test_timezones.py +++ b/tests/test_timezones.py @@ -14,6 +14,7 @@ from types import SimpleNamespace from zoneinfo import ZoneInfo from custom_components.nestquest.const import DEFAULT_HORIZON_DAYS +from custom_components.nestquest.core.settings import NestQuestSettings from custom_components.nestquest.dao_children import ChildrenDao from custom_components.nestquest.dao_instances import QuestInstancesDao from custom_components.nestquest.db import NestQuestDatabase @@ -147,7 +148,9 @@ def test_horizon_uses_newyork_local_date_after_local_midnight( import custom_components.nestquest as nestquest child_id = await _seed_daily(database, local_today.isoformat()) - await nestquest._run_horizon_materialization(hass, database) + await nestquest._run_horizon_materialization( + hass, database, NestQuestSettings() + ) records = await QuestInstancesDao(database).list_by_date_range( child_id, local_today.isoformat(), @@ -187,7 +190,9 @@ def test_horizon_uses_local_date_not_utc_when_host_ahead( import custom_components.nestquest as nestquest child_id = await _seed_daily(database, local_today.isoformat()) - await nestquest._run_horizon_materialization(hass, database) + await nestquest._run_horizon_materialization( + hass, database, NestQuestSettings() + ) records = await QuestInstancesDao(database).list_by_date_range( child_id, local_today.isoformat(), @@ -231,7 +236,9 @@ def test_horizon_resolves_spring_forward_day_without_off_by_one( requested_batches.append(requested) local_today = instant.date() child_id = await _seed_daily(database, local_today.isoformat()) - await nestquest._run_horizon_materialization(hass, database) + await nestquest._run_horizon_materialization( + hass, database, NestQuestSettings() + ) records = await QuestInstancesDao(database).list_by_date_range( child_id, local_today.isoformat(), @@ -280,7 +287,9 @@ def test_horizon_resolves_fall_back_day_without_off_by_one( requested_batches.append(requested) local_today = instant.date() child_id = await _seed_daily(database, local_today.isoformat()) - await nestquest._run_horizon_materialization(hass, database) + await nestquest._run_horizon_materialization( + hass, database, NestQuestSettings() + ) records = await QuestInstancesDao(database).list_by_date_range( child_id, local_today.isoformat(), -- 2.55.0 From d15a02fb48de8211ac5829da1ae7bdaf563a112d Mon Sep 17 00:00:00 2001 From: Joshua Date: Mon, 21 Sep 2026 15:51:47 -0400 Subject: [PATCH 4/4] Apply PR #144 review round 3: lifecycle tests for malformed-option fallback, comment/test hygiene Add LIFECYCLE tests that wire an entry via make_entry(options=...) and call async_setup_entry, covering the from_options_resilient fallback production actually uses (P2): a fully malformed options dict (horizon_days=True, day_rollover_time='99:99', morning_summary_time='25:00') succeeds with the day-rollover listener at the default 00:00 and the backfill over DEFAULT_HORIZON_DAYS; and a bad sibling (horizon_days=5, morning_summary_time='25:00') keeps horizon_days == 5. Swapping from_options_resilient for from_options in setup now fails these tests. Reword comments/tests describing review history rather than the code: __init__.py setup comment, test_core_settings P2-3/P2-1/P2-5 markers, the moved from_options horizon test, and the AST scanner docstrings. Test hygiene: move freeze_nestquest_clock into tests/conftest.py and use it from test_core_settings and test_timezones; simplify CORE_PKG to a plain import plus Path(mod.__file__).parent; move test_from_options_horizon_validates_and_defaults into test_core_settings. 1575 passed. --- custom_components/nestquest/__init__.py | 7 +- tests/conftest.py | 39 ++++++++- tests/test_core_settings.py | 110 +++++++++++------------- tests/test_lifecycle.py | 86 ++++++++++++++---- tests/test_timezones.py | 32 +------ 5 files changed, 163 insertions(+), 111 deletions(-) diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 7d01021..e8bd9fb 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -467,10 +467,9 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: # horizon materialization and the daily-rollover listener both # consume it, so core never reads HA config itself. A malformed # stored option falls back PER FIELD (via from_options_resilient) - # so a single bad sibling option cannot silently reset the user's - # horizon_days or day_rollover_time — the two fields core consumes - # — rather than the all-or-nothing reset a single try/except would - # cause (a malformed morning_summary_time used to wipe horizon_days). + # so a single bad sibling option cannot reset the user's + # horizon_days or day_rollover_time — the two fields core + # consumes — to their defaults. settings = NestQuestSettings.from_options_resilient(entry.options) remove_update_listener = entry.add_update_listener(_async_update_listener) remove_time_change_listener = _register_day_rollover_listener( diff --git a/tests/conftest.py b/tests/conftest.py index 8fdf59f..1e4b728 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -22,6 +22,7 @@ pytest -q``); it is never meant to run against a real HA install. from __future__ import annotations import asyncio +import datetime import importlib.util import inspect import sys @@ -916,4 +917,40 @@ def make_config_flow_entry(): def _factory(domain: str, entry_id: str = "existing"): return SimpleNamespace(domain=domain, entry_id=entry_id) - return _factory \ No newline at end of file + return _factory + + +def freeze_nestquest_clock( + monkeypatch, instant: datetime.datetime +) -> tuple[datetime.datetime, list]: + """Freeze the integration's ``datetime`` clock to a fixed aware instant. + + Only ``custom_components.nestquest``'s ``datetime`` binding is patched, + so the generation path's ``datetime.datetime.now`` reads this instant + (localized to whatever time zone it asks for) while every other module + and the test harness keep the real clock. Returns ``(frozen, requested)`` + where ``frozen`` is the instant normalized to UTC and ``requested`` is + the list of ``tzinfo`` objects the code under test passed to ``now`` — + so a caller can prove the generation path actually requested the + configured HA time zone rather than silently reading UTC. + """ + import custom_components.nestquest as nestquest + + frozen = instant.astimezone(datetime.timezone.utc) + requested: list = [] + + class _FrozenDatetime(datetime.datetime): + @classmethod + def now(cls, tz=None): + requested.append(tz) + if tz is None: + return frozen.replace(tzinfo=None) + return frozen.astimezone(tz) + + fake_module = SimpleNamespace( + datetime=_FrozenDatetime, + timedelta=datetime.timedelta, + date=datetime.date, + ) + monkeypatch.setattr(nestquest, "datetime", fake_module) + return frozen, requested \ No newline at end of file diff --git a/tests/test_core_settings.py b/tests/test_core_settings.py index c07f456..67a4b9c 100644 --- a/tests/test_core_settings.py +++ b/tests/test_core_settings.py @@ -24,11 +24,13 @@ import ast import datetime from dataclasses import FrozenInstanceError from pathlib import Path -from types import SimpleNamespace from typing import Any import pytest +import custom_components.nestquest.core as _core_mod +from conftest import freeze_nestquest_clock + from custom_components.nestquest.const import ( CONF_AFTERNOON_REMINDER_ENABLED, CONF_AFTERNOON_REMINDER_TIME, @@ -139,6 +141,29 @@ def test_from_options_horizon_absent_falls_back_to_default() -> None: ) +def test_from_options_horizon_validates_and_defaults() -> None: + """from_options returns the configured horizon, defaulting when absent/None. + + A valid horizon is read; an absent or None key falls back to the + default; and a malformed value RAISES (the integration's + ``from_options_resilient`` handles the per-field fall-back at setup, + not this strict path). + """ + assert NestQuestSettings.from_options({CONF_HORIZON_DAYS: 5}).horizon_days == 5 + assert ( + NestQuestSettings.from_options({}).horizon_days == DEFAULT_HORIZON_DAYS + ) + assert ( + NestQuestSettings.from_options( + {CONF_HORIZON_DAYS: None} + ).horizon_days + == DEFAULT_HORIZON_DAYS + ) + for bad in (True, "5", 0): + with pytest.raises(ValueError): + NestQuestSettings.from_options({CONF_HORIZON_DAYS: bad}) + + @pytest.mark.parametrize( "value", ["25:00", "00:60", "24:00", "9:5", "abc", "07:00:00", 700, ""], @@ -212,7 +237,7 @@ def test_from_options_strips_notify_target() -> None: # --------------------------------------------------------------------------- -# from_options_resilient: per-field fallback (P2-1). +# from_options_resilient: per-field fallback. # --------------------------------------------------------------------------- @@ -335,7 +360,7 @@ def test_direct_construction_validates() -> None: def test_direct_construction_rejects_none(field: str, kwargs: dict[str, Any]) -> None: """__post_init__ is STRICT: None is rejected for every field. - P2-5: the None->default mapping lives ONLY in from_options (via + The None->default mapping lives ONLY in from_options (via _option_value). A hand-built NestQuestSettings(field=None) must raise in __post_init__ rather than constructing and later blowing up horizon_window (timedelta(days=None) -> TypeError) or the rollover @@ -419,11 +444,7 @@ def test_horizon_window_non_default_horizon() -> None: # AST: no HA config reads under core/. # --------------------------------------------------------------------------- -CORE_PKG = Path( - __import__( - "custom_components.nestquest.core", fromlist=["__file__"] - ).__file__ -).parent +CORE_PKG = Path(_core_mod.__file__).parent def _is_homeassistant(module: str) -> bool: @@ -446,8 +467,8 @@ _FORBIDDEN_ATTRS = frozenset({"hass", "entry", "config_entry"}) #: Config-entry attribute reads — the shapes ``hass.config``, #: ``config_entry.options``, ``self.entry.options``, ``runtime.options``, #: ``entry.data``. Flagging the attribute ANYWHERE (regardless of root) -#: is what catches ``runtime.options``, which the old root-only check -#: missed. No core module currently reads any of these attributes. +#: catches ``runtime.options``, whose root name is not a forbidden +#: identifier. No core module currently reads any of these attributes. _CONFIG_ATTRS = frozenset({"config", "options", "data"}) #: Dynamic config-entry reads via getattr/hasattr with a string argument. @@ -475,8 +496,8 @@ def _scan_source_for_ha_config( ) -> list[str]: """Return offender strings for HA-coupling in ``source``. - Flags (each closes a bypass the old root-only ``hass``/``entry`` - chain check missed): + Flags (each closes a bypass a root-only ``hass``/``entry`` chain + check would not catch): - any ``import homeassistant`` / ``from homeassistant ...``; - any ``importlib.import_module('homeassistant...')`` (a string-based @@ -489,14 +510,14 @@ def _scan_source_for_ha_config( bare-Name check misses keyword arguments); - any attribute access whose ``attr`` is ``hass``/``entry``/ ``config_entry`` (catches ``self.hass``, ``self.entry``, - ``obj.config_entry`` — root names the old check could not see); + ``obj.config_entry`` — root names a root-only check could not see); - any attribute access whose ``attr`` is ``config``/``options``/ ``data`` — these are Home Assistant config-entry attributes with no legitimate core use (``.config`` reads ``hass.config``, ``.options`` reads ``config_entry.options``, ``.data`` reads ``config_entry.data``); flagging the attribute ANYWHERE (regardless of root) is what catches - ``runtime.options``, which the old root-only check missed. No core - module currently reads any of these attributes; + ``runtime.options``, whose root name is not a forbidden identifier. + No core module currently reads any of these attributes; - any ``getattr``/``hasattr`` call whose string argument is ``'config'``/``'options'``/``'data'`` (catches the dynamic read ``getattr(hass, 'config')`` the static attribute check cannot see). @@ -604,13 +625,13 @@ def test_core_modules_have_no_ha_config_reads() -> None: the HA-object attributes ``.hass``/``.entry``/``.config_entry``, the config-entry attribute reads ``.config``/``.options``/``.data``, and dynamic ``getattr``/``hasattr`` reads of them — so a chain rooted at - ``self`` or ``runtime`` (which the old root-only check missed) is now - caught. ``.config``/``.options``/``.data`` are banned because they are - Home Assistant config-entry attributes with no legitimate core use: - core receives the resolved values via the explicit - :class:`NestQuestSettings` object the integration builds from - ``entry.options``, so any read of those attributes in ``core/`` is a - config-entry coupling the settings object exists to remove. + ``self`` or ``runtime`` (whose root name is not a forbidden + identifier) is caught. ``.config``/``.options``/``.data`` are banned + because they are Home Assistant config-entry attributes with no + legitimate core use: core receives the resolved values via the + explicit :class:`NestQuestSettings` object the integration builds + from ``entry.options``, so any read of those attributes in ``core/`` + is a config-entry coupling the settings object exists to remove. Docstring mentions pass: AST only sees code. """ py_files = sorted(CORE_PKG.rglob("*.py")) @@ -625,11 +646,11 @@ def test_core_modules_have_no_ha_config_reads() -> None: def test_ast_scan_flags_each_bypass_shape() -> None: - """Negative control: the scanner flags each bypass shape the old check missed. + """Negative control: the scanner flags each bypass a root-only check misses. - The old scan only flagged chains rooted at a literal name ``hass``/ - ``entry``. Each snippet below is a shape it missed; the new scanner - must flag every one. + A root-only scan flags chains rooted at a literal name ``hass``/ + ``entry``. Each snippet below is a shape such a scan would miss; + this scanner must flag every one. """ snippets: dict[str, str] = { "self.hass.config": "x = self.hass.config\n", @@ -809,49 +830,18 @@ async def test_materialize_over_non_default_horizon(hass, tmp_path) -> None: await database.close() -def _freeze_nestquest_clock( - monkeypatch, instant: datetime.datetime -) -> None: - """Freeze the integration's ``datetime`` clock to a fixed aware instant. - - Only ``custom_components.nestquest``'s ``datetime`` binding is patched, - so ``_run_horizon_materialization``'s ``datetime.datetime.now`` reads - this instant (localized to ``hass.config.time_zone``) while the test - harness and core keep the real clock. Mirrors the helper in - test_timezones.py. - """ - import custom_components.nestquest as nestquest - - frozen = instant.astimezone(datetime.timezone.utc) - - class _FrozenDatetime(datetime.datetime): - @classmethod - def now(cls, tz=None): - if tz is None: - return frozen.replace(tzinfo=None) - return frozen.astimezone(tz) - - fake_module = SimpleNamespace( - datetime=_FrozenDatetime, - timedelta=datetime.timedelta, - date=datetime.date, - ) - monkeypatch.setattr(nestquest, "datetime", fake_module) - - async def test_integration_path_threads_settings( hass, tmp_path, monkeypatch ) -> None: """The integration path threads ``settings`` through horizon_window + rollover. - P2-3: ``_run_horizon_materialization`` now calls + ``_run_horizon_materialization`` calls ``settings.horizon_window(today)`` (so the helper is real and shared, not recomputed inline), and ``_register_day_rollover_listener`` registers at ``settings.day_rollover_hour_minute``. This runs the INTEGRATION path with ``horizon_days=3`` and ``day_rollover_time='03:30'`` and asserts the resulting instance - window matches [today, today+3] AND the registered hour/minute are 3/30 - — a test the requirement-level core-only check could not fail. + window matches [today, today+3] AND the registered hour/minute are 3/30. """ import custom_components.nestquest as nestquest from custom_components.nestquest.core.dao_children import ChildrenDao @@ -869,7 +859,7 @@ async def test_integration_path_threads_settings( # Freeze the integration clock so "today" is deterministic (the # hass fixture's config.time_zone is UTC, so 2026-03-01 12:00 UTC # localizes to 2026-03-01). - _freeze_nestquest_clock( + freeze_nestquest_clock( monkeypatch, datetime.datetime(2026, 3, 1, 12, 0, tzinfo=datetime.timezone.utc) ) diff --git a/tests/test_lifecycle.py b/tests/test_lifecycle.py index 8955c3f..341522e 100644 --- a/tests/test_lifecycle.py +++ b/tests/test_lifecycle.py @@ -12,6 +12,7 @@ from custom_components.nestquest.const import ( CONF_ADMIN_USER_IDS, CONF_DAY_ROLLOVER_TIME, CONF_HORIZON_DAYS, + CONF_MORNING_SUMMARY_TIME, DEFAULT_HORIZON_DAYS, SERVICE_REGENERATE, DOMAIN as DOMAIN_CONST, @@ -526,29 +527,78 @@ async def test_setup_backfills_configured_horizon_days(hass, make_entry) -> None ).list_by_date_range(child_id, beyond, beyond_end) == [] -def test_from_options_horizon_validates_and_defaults() -> None: - """from_options returns the configured horizon, defaulting when absent/None. +async def test_setup_succeeds_with_fully_malformed_options( + hass, make_entry +) -> None: + """A fully malformed options dict falls back per field and setup succeeds. - Repointed from the removed ``_configured_horizon_days`` shim: a valid - horizon is read, an absent or None key falls back to the default, and - a malformed value RAISES (the integration's ``from_options_resilient`` - handles the per-field fall-back at setup, not this strict path). + ``horizon_days=True``, ``day_rollover_time='99:99'`` and + ``morning_summary_time='25:00'`` are all invalid; the integration's + ``from_options_resilient`` falls each back to its default, so setup + SUCCEEDS, the day-rollover listener registers at the default hour 0 + minute 0, and the startup backfill materializes the default horizon + window. """ - from custom_components.nestquest.core.settings import NestQuestSettings + import datetime - assert NestQuestSettings.from_options({CONF_HORIZON_DAYS: 5}).horizon_days == 5 - assert ( - NestQuestSettings.from_options({}).horizon_days == DEFAULT_HORIZON_DAYS + from custom_components.nestquest.dao_instances import QuestInstancesDao + from custom_components.nestquest.db import NestQuestDatabase + from custom_components.nestquest.migrations import apply_migrations + from custom_components.nestquest.store import async_get_db_path + + db_path = await async_get_db_path(hass) + pre = NestQuestDatabase(hass.async_add_executor_job) + await pre.open(db_path) + await apply_migrations(pre) + child_id = await _seed_daily_child_and_definition(pre, _local_today(hass)) + await pre.close() + + entry = _wire( + make_entry( + options={ + CONF_HORIZON_DAYS: True, + CONF_DAY_ROLLOVER_TIME: "99:99", + CONF_MORNING_SUMMARY_TIME: "25:00", + } + ), + hass.registry, ) - assert ( - NestQuestSettings.from_options( - {CONF_HORIZON_DAYS: None} - ).horizon_days - == DEFAULT_HORIZON_DAYS + assert await async_setup_entry(hass, entry) is True + + # The day-rollover listener fell back to the default 00:00. + registration = hass.time_change.registrations[0] + assert registration["hour"] == 0 + assert registration["minute"] == 0 + + # The startup backfill used the default horizon window. + today = _local_today(hass) + end = (today + datetime.timedelta(days=DEFAULT_HORIZON_DAYS)).isoformat() + records = await QuestInstancesDao( + entry.runtime_data.database + ).list_by_date_range(child_id, today.isoformat(), end) + assert len(records) == DEFAULT_HORIZON_DAYS + 1 + + +async def test_setup_keeps_horizon_when_sibling_option_is_bad( + hass, make_entry +) -> None: + """A malformed sibling option does not reset the configured horizon. + + ``horizon_days=5`` is valid while ``morning_summary_time='25:00'`` is + invalid; ``from_options_resilient`` falls back ONLY the bad sibling, + so the entry's resolved settings keep ``horizon_days == 5``. + """ + entry = _wire( + make_entry( + options={ + CONF_HORIZON_DAYS: 5, + CONF_MORNING_SUMMARY_TIME: "25:00", + } + ), + hass.registry, ) - for bad in (True, "5", 0): - with pytest.raises(ValueError): - NestQuestSettings.from_options({CONF_HORIZON_DAYS: bad}) + assert await async_setup_entry(hass, entry) is True + assert entry.runtime_data.settings.horizon_days == 5 async def test_regenerate_service_registered_and_materializes( diff --git a/tests/test_timezones.py b/tests/test_timezones.py index 09d8271..0d1dce5 100644 --- a/tests/test_timezones.py +++ b/tests/test_timezones.py @@ -13,6 +13,7 @@ import datetime from types import SimpleNamespace from zoneinfo import ZoneInfo +from conftest import freeze_nestquest_clock from custom_components.nestquest.const import DEFAULT_HORIZON_DAYS from custom_components.nestquest.core.settings import NestQuestSettings from custom_components.nestquest.dao_children import ChildrenDao @@ -54,35 +55,10 @@ def _freeze_clock( ) -> tuple[datetime.datetime, list]: """Freeze the integration's ``datetime`` clock to a fixed aware instant. - Only ``custom_components.nestquest``'s ``datetime`` binding is patched, so - the generation path's "today" reads this instant (localized to whatever - time zone it asks for) while every other module and the test harness keep - the real clock. Returns ``(frozen, requested)`` where ``frozen`` is the - instant normalized to UTC and ``requested`` is the list of ``tzinfo`` - objects the code under test passed to ``now`` — so a test can prove the - generation path actually requested the configured HA time zone rather - than silently reading UTC. + Thin wrapper over :func:`conftest.freeze_nestquest_clock` kept for the + ``(frozen, requested)`` return shape the timezone assertions read. """ - import custom_components.nestquest as nestquest - - frozen = instant.astimezone(datetime.timezone.utc) - requested: list = [] - - class _FrozenDatetime(datetime.datetime): - @classmethod - def now(cls, tz=None): - requested.append(tz) - if tz is None: - return frozen.replace(tzinfo=None) - return frozen.astimezone(tz) - - fake_module = SimpleNamespace( - datetime=_FrozenDatetime, - timedelta=datetime.timedelta, - date=datetime.date, - ) - monkeypatch.setattr(nestquest, "datetime", fake_module) - return frozen, requested + return freeze_nestquest_clock(monkeypatch, instant) def _assert_requested_ha_timezone(requested, time_zone: str) -> None: -- 2.55.0