fix: reject duplicates, return consistent snapshot, test post-write rollback (NQ-67-01..03)

custom_components/nestquest/dao_rules.py

@@ -163,6 +163,22 @@ class QuestDefinitionWindowRecord:
     due_time: str | None
 
 
+@dataclass(frozen=True)
+class QuestDefinitionSnapshot:
+    """A definition plus its assignees and windows, read atomically.
+
+    Returned by :meth:`QuestDefinitionsDao.create_with_rule_and_windows`
+    with the assignees and windows read inside the SAME transaction that
+    inserted them, so the snapshot is a consistent creation-time view —
+    a concurrent assignee or window mutation cannot change it between
+    the write and the read.
+    """
+
+    definition: QuestDefinitionRecord
+    assignees: list[ChildRecord]
+    windows: list[QuestDefinitionWindowRecord]
+
+
 _RULE_COLUMNS = (
     "id, rule_type, interval, weekday_set, day_of_month, nth_weekday, "
     "month, start_date, end_date"
@@ -644,7 +660,7 @@ class QuestDefinitionsDao:
         *,
         description: str | None = None,
         icon: str | None = None,
-    ) -> QuestDefinitionRecord:
+    ) -> QuestDefinitionSnapshot:
         """Insert a rule, definition, assignees and windows atomically.
 
         The whole write — the schedule rule row, the definition row,
@@ -655,6 +671,11 @@ class QuestDefinitionsDao:
         assignee or window, and a rejected create leaves no partial
         rows.
 
+        Returns a :class:`QuestDefinitionSnapshot` whose assignees and
+        windows are read back inside the same transaction, so the
+        returned view cannot be raced by a concurrent assignee/window
+        mutation after the write.
+
         The rule arrives as pre-validated storage fields (mapped from a
         :class:`~.recurrence.ScheduleRule` via
         :func:`schedule_rule_to_storage`); its dates are re-validated
@@ -696,8 +717,14 @@ class QuestDefinitionsDao:
                         (definition_id, window, due_time),
                     )
                 definition = await self.get(definition_id)
+                assignees = await self.list_assignees(definition_id)
+                window_records = await self.list_windows(definition_id)
         assert definition is not None
-        return definition
+        return QuestDefinitionSnapshot(
+            definition=definition,
+            assignees=assignees,
+            windows=window_records,
+        )
 
     async def _validate_rule_exists(self, schedule_rule_id: int) -> None:
         """Raise ValueError unless the rule exists.

custom_components/nestquest/quest_definitions.py

@@ -16,11 +16,15 @@ Validation policy mirrors :mod:`.children`:
 - ``assignee_child_ids`` is non-empty; every id is a plain int (bools
   and floats rejected, since SQLite would bind them onto a real child)
   and must reference an existing ACTIVE child (the DAO enforces the
-  active/exists check inside the same transaction).
+  active/exists check inside the same transaction).  Duplicate ids are
+  rejected here, before the composite primary key could surface them as
+  an :class:`sqlite3.IntegrityError`.
 - ``windows`` is non-empty; each entry is a window name from
   :data:`~.const.QUEST_WINDOWS` — either a bare name (no due time) or a
   ``(name, due_time)`` pair whose due time is a strict 24-hour HH:MM
-  string.
+  string.  Duplicate window names are rejected here, before the
+  composite primary key could surface them as an
+  :class:`sqlite3.IntegrityError`.
 
 The rule, definition, assignees and windows are persisted in ONE
 transaction (the DAO's :meth:`~.dao_rules.QuestDefinitionsDao.create_with_rule_and_windows`),
@@ -113,6 +117,10 @@ def _validate_assignee_ids(value: object) -> list[int]:
                 f"got {entry!r}"
             )
         ids.append(entry)
+    if len(set(ids)) != len(ids):
+        raise ValueError(
+            "assignee_child_ids must not contain duplicate child ids"
+        )
     return ids
 
 
@@ -135,6 +143,7 @@ def _normalize_windows(
     if not value:
         raise ValueError("windows must not be empty")
     normalized: list[tuple[str, str | None]] = []
+    seen: set[str] = set()
     for index, entry in enumerate(value):
         if isinstance(entry, str):
             name: object = entry
@@ -149,6 +158,11 @@ def _normalize_windows(
         _validate_window(name)
         if due_time is not None:
             _validate_time(due_time, "due_time")
+        if name in seen:
+            raise ValueError(
+                f"windows must not contain duplicate window {name!r}"
+            )
+        seen.add(name)
         normalized.append((name, due_time))
     return normalized
 
@@ -168,8 +182,10 @@ async def create_quest_definition(
     Validates the arguments up front, then persists the schedule rule,
     the definition, its assignees and its windows in one transaction via
     the DAO.  Returns the stored definition together with its decoded
-    rule, assignees and windows.  Raises ValueError on any rejected
-    argument; a rejected or failed create persists nothing.
+    rule, assignees and windows — all read back inside that same
+    transaction, so the returned bundle is a consistent creation
+    snapshot.  Raises ValueError on any rejected argument; a rejected or
+    failed create persists nothing.
     """
     name = _validate_text(title, "title", required=True)
     assert name is not None
@@ -182,7 +198,7 @@ async def create_quest_definition(
 
     storage = schedule_rule_to_storage(rule)
     dao = QuestDefinitionsDao(database)
-    definition = await dao.create_with_rule_and_windows(
+    snapshot = await dao.create_with_rule_and_windows(
         name,
         storage,
         _now_stamp(),
@@ -192,11 +208,9 @@ async def create_quest_definition(
         icon=icon_value,
     )
     decoded_rule = schedule_rule_from_storage(storage)
-    assignees = await dao.list_assignees(definition.id)
-    window_records = await dao.list_windows(definition.id)
     return CreatedQuestDefinition(
-        definition=definition,
+        definition=snapshot.definition,
         rule=decoded_rule,
-        assignees=assignees,
-        windows=window_records,
+        assignees=snapshot.assignees,
+        windows=snapshot.windows,
     )

tests/test_quest_definitions.py

@@ -10,6 +10,7 @@ from custom_components.nestquest.dao_children import ChildrenDao
 from custom_components.nestquest.dao_rules import (
     QuestDefinitionsDao,
     ScheduleRulesDao,
+    schedule_rule_to_storage,
 )
 from custom_components.nestquest.db import NestQuestDatabase
 from custom_components.nestquest.migrations import apply_migrations
@@ -213,6 +214,23 @@ def test_create_rejects_non_int_assignee(tmp_path, bad) -> None:
     _with_db(tmp_path, "create-bad-assignee.db")(_body)
 
 
+def test_create_rejects_duplicate_assignees(tmp_path) -> None:
+    async def _body(database, children, rules, definitions, child):
+        with pytest.raises(
+            ValueError, match="assignee_child_ids must not contain duplicate"
+        ):
+            await create_quest_definition(
+                database,
+                "Brush teeth",
+                _daily_rule(),
+                [child.id, child.id],
+                ["morning"],
+            )
+        return None
+
+    _with_db(tmp_path, "create-duplicate-assignees.db")(_body)
+
+
 def test_create_rejects_inactive_child(tmp_path) -> None:
     async def _body(database, children, rules, definitions, child):
         inactive = await children.create("Bo", NOW)
@@ -265,6 +283,33 @@ def test_create_rejects_invalid_window(tmp_path, bad) -> None:
     _with_db(tmp_path, "create-bad-window.db")(_body)
 
 
+def test_create_rejects_duplicate_windows(tmp_path) -> None:
+    async def _body(database, children, rules, definitions, child):
+        with pytest.raises(
+            ValueError, match="windows must not contain duplicate"
+        ):
+            await create_quest_definition(
+                database,
+                "Brush teeth",
+                _daily_rule(),
+                [child.id],
+                ["morning", "morning"],
+            )
+        with pytest.raises(
+            ValueError, match="windows must not contain duplicate"
+        ):
+            await create_quest_definition(
+                database,
+                "Brush teeth",
+                _daily_rule(),
+                [child.id],
+                [("morning", "09:00"), ("morning", "10:00")],
+            )
+        return None
+
+    _with_db(tmp_path, "create-duplicate-windows.db")(_body)
+
+
 @pytest.mark.parametrize("bad", ["9:30", "25:00", "09:60", "0900"])
 def test_create_rejects_malformed_due_time(tmp_path, bad) -> None:
     async def _body(database, children, rules, definitions, child):
@@ -351,3 +396,36 @@ def test_rejected_create_writes_nothing(tmp_path) -> None:
         return None
 
     _with_db(tmp_path, "create-rejected-nothing.db")(_body)
+
+
+def test_post_write_failure_rolls_back_everything(tmp_path) -> None:
+    """A failure after some writes rolls all four tables back together.
+
+    Deterministic post-write trigger: the DAO method inserts the rule,
+    the definition and the assignee rows, THEN validates the window name
+    and raises ValueError for a window outside const.QUEST_WINDOWS.
+    That failure lands after three writes inside the transaction, so the
+    rule, definition and assignee rows must all roll back — no partial
+    rows survive in any of the four tables.
+    """
+    async def _body(database, children, rules, definitions, child):
+        storage = schedule_rule_to_storage(_daily_rule())
+        dao = QuestDefinitionsDao(database)
+        with pytest.raises(ValueError, match="window must be one of"):
+            await dao.create_with_rule_and_windows(
+                "Brush teeth",
+                storage,
+                NOW,
+                [child.id],
+                [("noon", None)],
+            )
+        # All four tables are empty: the earlier rule, definition and
+        # assignee writes were rolled back, and no window row was added.
+        assert await rules.get(1) is None
+        assert await definitions.get(1) is None
+        assert await definitions.list_active() == []
+        assert await definitions.list_assignees(1) == []
+        assert await definitions.list_windows(1) == []
+        return None
+
+    _with_db(tmp_path, "post-write-rollback.db")(_body)