Widen instance key to (definition_id, child_id, due_date, window) (D-008)

Per IMPLEMENTATION-ORDER.md 1.2: quest_instances gains a NOT NULL
window column (CHECK-constrained to the const window spellings) and
the table-level UNIQUE constraint widens to the full fan-out tuple.

- schema.py: instance table body extracted to QUEST_INSTANCES_TABLE_SQL
  shared with the rebuild migration; uniqueness docstring rewritten for
  the widened key.
- migrations.py: migration 5 rebuilds a pre-window quest_instances via
  the canonical procedure; migrated rows are pinned to the documented
  default 'morning' window (pre-window rows were whole-day obligations;
  future dates are regenerated with real windows by materialization),
  preserving ids so completion_events keep pointing at the same
  instances; PRAGMA foreign_key_check verified inside the transaction.
- dao_instances.py: upsert takes required keyword window and is
  idempotent on the widened tuple; the immutable-once-completed check
  and the read-back get() are keyed on the full tuple; window spelling
  validated via dao_rules._validate_window.
- tests: schema shape (window column, widened autoindex columns),
  twice-daily and shared-assignee fan-out cases at both the DDL and DAO
  layers (incl. idempotent re-runs), cross-child same-key allowance,
  window CHECK/NOT NULL constraints, v2 seed frozen to the true
  version-2 shapes and its migration assertions widened.

custom_components/nestquest/dao_instances.py

@@ -15,13 +15,13 @@ event.  A package-wide guard test (bottom of the test module) keeps
 mutation SQL for this table out of every other module.
 
 Upsert semantics: ``QuestInstancesDao.upsert`` is idempotent on
-(definition_id, due_date) — re-running materialization for a date
-whose instance already exists updates nothing that matters
-(child_id/due_time are refreshed only in the degenerate re-generation
-case where they changed) and never duplicates a row.  The schema's
-UNIQUE(definition_id, due_date) is the last line of defense.  The
-upsert refuses a child that is not an assignee of the definition
-(D-008 multi-assignee model).
+(definition_id, child_id, due_date, window) — re-running
+materialization for a tuple whose instance already exists refreshes
+only the generation-time snapshot columns and never duplicates a row.
+The schema's UNIQUE(definition_id, child_id, due_date, window) is the
+last line of defense.  The upsert refuses a child that is not an
+assignee of the definition and a window outside ``const.QUEST_WINDOWS``
+(D-008 multi-assignee, multi-window model).
 
 ``delete_future_uncompleted`` implements the Feature 06/07 rule that
 reassignment and schedule edits regenerate future instances: it
@@ -37,7 +37,7 @@ import datetime
 from dataclasses import dataclass
 
 from .dao_children import _connection_lock
-from .dao_rules import _validate_date
+from .dao_rules import _validate_date, _validate_window
 from .db import NestQuestDatabase
 
 #: Timestamp policy for completion_events.occurred_at: strict UTC
@@ -90,6 +90,7 @@ class QuestInstanceRecord:
     id: int
     definition_id: int
     child_id: int
+    window: str
     due_date: str
     due_time: str | None
     generated_at: str
@@ -110,7 +111,7 @@ class CompletionEventRecord:
 
 
 _INSTANCE_COLUMNS = (
-    "id, definition_id, child_id, due_date, due_time, generated_at"
+    "id, definition_id, child_id, window, due_date, due_time, generated_at"
 )
 _EVENT_COLUMNS = (
     "id, instance_id, child_id, event_type, actor_source, actor_user_id, "
@@ -128,9 +129,10 @@ def _instance_from_row(row: tuple) -> QuestInstanceRecord:
         id=row[0],
         definition_id=row[1],
         child_id=row[2],
-        due_date=row[3],
-        due_time=row[4],
-        generated_at=row[5],
+        window=row[3],
+        due_date=row[4],
+        due_time=row[5],
+        generated_at=row[6],
     )
 
 
@@ -160,31 +162,40 @@ class QuestInstancesDao:
         due_date: str,
         generated_at: str,
         *,
+        window: str,
         due_time: str | None = None,
     ) -> QuestInstanceRecord:
-        """Create the dated instance for (definition_id, due_date) idempotently.
+        """Create the dated instance for the widened key idempotently.
 
-        Running materialization twice for the same definition/date
-        yields ONE row: the ON CONFLICT path refreshes the
-        generation-time snapshot columns (child_id, due_time) and
-        regenerates the stamp, but can never create a duplicate — the
-        schema UNIQUE(definition_id, due_date) backs this up.
+        The instance key is (definition_id, child_id, due_date, window)
+        (D-008): running materialization twice for the same tuple
+        yields ONE row — the ON CONFLICT path refreshes the
+        generation-time snapshot columns (due_time) and regenerates the
+        stamp, but can never create a duplicate.  The schema's
+        UNIQUE(definition_id, child_id, due_date, window) backs this
+        up, so the twice-daily case (two windows, one child, one date)
+        yields two rows and the shared case (three assignees, one
+        window) yields three.
 
         Guardrails enforced here:
 
         - ``due_date`` must be TODAY or later: instances are never
           generated in the past (Feature 07).  The caller's
           materialization horizon is future-dated by definition.
+        - ``window`` must be one of ``const.QUEST_WINDOWS``.
         - An existing instance WITH a completion event is IMMUTABLE:
           the conflict path must not rewrite its snapshot columns
           (Feature 07/08).  The upsert refuses with ValueError in that
           case — silently no-oping would mask a materialization bug.
+        - The child must be an assignee of the definition (D-008
+          multi-assignee model).
 
         The definition must exist (validated atomically under the
         connection lock so a concurrent definition deactivation or
         child deletion cannot slip between check and insert).
         """
         _validate_date(due_date, "due_date")
+        _validate_window(window)
         # Fail fast on obviously-past dates BEFORE queuing on the
         # connection lock (a nice early error); the AUTHORITATIVE
         # check runs after the lock is acquired, because a caller can
@@ -227,51 +238,56 @@ class QuestInstancesDao:
                         f"assigned to child {child_id}; instance must "
                         "carry an assigned child"
                     )
-                child = await self._database.fetch_one(
-                    "SELECT 1 FROM children WHERE id = ?", (child_id,)
-                )
-                if child is None:
-                    raise ValueError(f"child {child_id} does not exist")
                 existing = await self._database.fetch_one(
                     "SELECT 1 FROM completion_events "
                     "WHERE instance_id = (SELECT id FROM quest_instances "
-                    "WHERE definition_id = ? AND due_date = ?)",
-                    (definition_id, due_date),
+                    "WHERE definition_id = ? AND child_id = ? "
+                    "AND due_date = ? AND window = ?)",
+                    (definition_id, child_id, due_date, window),
                 )
                 if existing is not None:
                     raise ValueError(
                         f"instance for (definition {definition_id}, "
-                        f"due_date {due_date!r}) already has a completion "
+                        f"child {child_id}, due_date {due_date!r}, "
+                        f"window {window!r}) already has a completion "
                         "event and is immutable; it cannot be regenerated"
                     )
                 await self._database.execute(
                     "INSERT INTO quest_instances (definition_id, child_id, "
-                    "due_date, due_time, generated_at) "
-                    "VALUES (?, ?, ?, ?, ?) "
-                    "ON CONFLICT (definition_id, due_date) DO UPDATE SET "
-                    "child_id = excluded.child_id, "
+                    "window, due_date, due_time, generated_at) "
+                    "VALUES (?, ?, ?, ?, ?, ?) "
+                    "ON CONFLICT (definition_id, child_id, due_date, "
+                    "window) DO UPDATE SET "
                     "due_time = excluded.due_time, "
                     "generated_at = excluded.generated_at",
                     (
                         definition_id,
                         child_id,
+                        window,
                         due_date,
                         due_time,
                         generated_at,
                     ),
                 )
-                instance = await self.get(definition_id, due_date)
+                instance = await self.get(
+                    definition_id, child_id, due_date, window
+                )
         assert instance is not None
         return instance
 
     async def get(
-        self, definition_id: int, due_date: str
+        self,
+        definition_id: int,
+        child_id: int,
+        due_date: str,
+        window: str,
     ) -> QuestInstanceRecord | None:
-        """Return the instance for (definition_id, due_date), or None."""
+        """Return the instance for the widened key tuple, or None."""
         row = await self._database.fetch_one(
             f"SELECT {_INSTANCE_COLUMNS} FROM quest_instances "
-            "WHERE definition_id = ? AND due_date = ?",
-            (definition_id, due_date),
+            "WHERE definition_id = ? AND child_id = ? AND due_date = ? "
+            "AND window = ?",
+            (definition_id, child_id, due_date, window),
         )
         return _instance_from_row(row) if row is not None else None
 

custom_components/nestquest/migrations.py

@@ -24,9 +24,12 @@ names before anything else assumes the new model.  Migration 3 brings
 pre-D-008 databases to the multi-assignee model: it creates
 ``quest_definition_assignees`` and, when ``quest_definitions`` still
 carries a single ``child_id`` column, rebuilds the table without it,
-copying each definition's assignee across.  Each callable migration
-runs inside its own transaction together with its version stamp, so a
-crash mid-step rolls the statements and the stamp back together.
+copying each definition's assignee across.  Migration 4 adds the
+windows table and migration 5 rebuilds ``quest_instances`` onto the
+widened (definition_id, child_id, due_date, window) key.  Each callable
+migration runs inside its own transaction together with its version
+stamp, so a crash mid-step rolls the statements and the stamp back
+together.
 
 Migration entries are either an ordered sequence of SQL statements or
 an async callable taking ``(database, target_version)``.  Callables
@@ -71,6 +74,7 @@ from typing import Union
 from .db import NestQuestDatabase
 from .schema import (
     QUEST_DEFINITIONS_TABLE_SQL,
+    QUEST_INSTANCES_TABLE_SQL,
     SCHEMA_V1_STATEMENTS,
     SCHEMA_V1_QUEST_DEFINITION_ASSIGNEES_DDL,
     SCHEMA_V1_QUEST_DEFINITION_WINDOWS_DDL,
@@ -147,6 +151,14 @@ async def _table_has_column(
     return any(row[1] == column for row in rows)
 
 
+#: Window assigned to instances migrated from the pre-window instance
+#: shape (D-008).  Pre-window rows were generated for whole-day
+#: obligations; the morning window is the documented default landing
+#: spot, and future dates are regenerated with real window declarations
+#: by materialization (delete_future_uncompleted + re-run).
+LEGACY_MIGRATED_WINDOW = "morning"
+
+
 async def _rename_legacy_task_tables(
     database: NestQuestDatabase, target_version: int
 ) -> None:
@@ -250,6 +262,70 @@ async def _add_definition_assignees(
         await database.execute("PRAGMA foreign_keys = ON")
 
 
+async def _widen_instance_key(
+    database: NestQuestDatabase, target_version: int
+) -> None:
+    """Bring ``quest_instances`` to the D-008 instance key.
+
+    Databases whose instance table still carries the pre-window shape
+    (no ``window`` column, UNIQUE on (definition_id, due_date)) are
+    rebuilt via SQLite's canonical procedure into the widened shape —
+    UNIQUE (definition_id, child_id, due_date, window) — with every
+    migrated row landing in :data:`LEGACY_MIGRATED_WINDOW` so existing
+    ids (and the completion events referencing them) survive intact.
+    The old rows were unique per (definition, due_date), so pinning one
+    window cannot collide with the widened key.  Fresh databases whose
+    table already matches the canonical shape only stamp.
+
+    The rebuild runs with ``foreign_keys = OFF`` (dropping the old
+    table under enforcement would fire the implicit DELETE and violate
+    the ``completion_events`` foreign key) and verifies
+    ``PRAGMA foreign_key_check`` inside the transaction before the
+    stamp, so a corrupt copy aborts the whole migration.
+    """
+    legacy_shape = not await _table_has_column(
+        database, "quest_instances", "window"
+    )
+    await database.execute("PRAGMA foreign_keys = OFF")
+    try:
+        async with database.transaction():
+            if legacy_shape:
+                await database.execute(
+                    "CREATE TABLE quest_instances_rebuilt "
+                    f"{QUEST_INSTANCES_TABLE_SQL}"
+                )
+                await database.execute(
+                    "INSERT INTO quest_instances_rebuilt "
+                    "(id, definition_id, child_id, window, due_date, "
+                    "due_time, generated_at) "
+                    "SELECT id, definition_id, child_id, ?, due_date, "
+                    "due_time, generated_at "
+                    "FROM quest_instances ORDER BY id",
+                    (LEGACY_MIGRATED_WINDOW,),
+                )
+                await database.execute("DROP TABLE quest_instances")
+                await database.execute(
+                    "ALTER TABLE quest_instances_rebuilt "
+                    "RENAME TO quest_instances"
+                )
+                violations = await database.fetch_all(
+                    "PRAGMA foreign_key_check"
+                )
+                if violations:
+                    raise RuntimeError(
+                        "instance rebuild produced foreign-key "
+                        f"violations: {violations!r}"
+                    )
+                LOGGER.info(
+                    "Widened quest_instances key; migrated rows pinned "
+                    "to the %r window",
+                    LEGACY_MIGRATED_WINDOW,
+                )
+            await stamp_schema_version(database, target_version)
+    finally:
+        await database.execute("PRAGMA foreign_keys = ON")
+
+
 #: The ordered migration list.  Append-only: never edit an applied
 #: entry, add the next one instead.  (Migration 1's content was
 #: rewritten pre-release per D-007 — no version 1 database shipped.)
@@ -261,6 +337,9 @@ MIGRATIONS: Sequence[MigrationStep] = [
     # CREATE IF NOT EXISTS suffices for both fresh databases (which
     # already made it in migration 1) and pre-window databases.
     list(SCHEMA_V1_QUEST_DEFINITION_WINDOWS_DDL),
+    # Migration 5 (D-008): rebuild quest_instances onto the widened
+    # (definition_id, child_id, due_date, window) key.
+    _widen_instance_key,
 ]
 
 

custom_components/nestquest/schema.py

@@ -109,25 +109,31 @@ status column.  An instance's current state derives from the latest row
 in ``completion_events`` (Feature 08), so materialization (Feature 07)
 can insert instances without knowing anything about completion and the
 append-only log stays the single source of truth.  ``definition_id``,
-``due_date`` and ``child_id`` snapshot the materialization-time facts
-("why this instance exists"): reassignment and rule edits change future
-instances only and never rewrite these rows.  ``due_time`` snapshots
-the definition's optional due time at generation for the same reason —
-the instance keeps its generation-time value even if the definition is
-edited later.  ``generated_at`` is a caller-set UTC ISO-8601 timestamp
-per the module timestamp policy (no DB default, so a generation batch
-stamps one coherent time).
+``child_id``, ``due_date`` and ``window`` snapshot the
+materialization-time facts ("why this instance exists"): assignment
+edits and rule edits change future instances only and never rewrite
+these rows.  ``window`` records which declared day window produced the
+instance (D-008), CHECK-constrained to the same three spellings
+``const`` owns.  ``due_time`` snapshots the window's optional due time
+at generation for the same reason — the instance keeps its
+generation-time value even if the definition is edited later.
+``generated_at`` is a caller-set UTC ISO-8601 timestamp per the module
+timestamp policy (no DB default, so a generation batch stamps one
+coherent time).
 
-Uniqueness: the (definition_id, due_date) pair is declared as a
-table-level ``UNIQUE`` constraint rather than a separate ``CREATE
-UNIQUE INDEX`` statement.  SQLite materializes it as an implicit unique
-index (``sqlite_autoindex_quest_instances_1``) — the required "unique
-index prevents duplicate instances" — and the constraint form cannot be
-silently dropped the way a standalone index can, so the duplicate door
-stays shut.  Idempotent materialization keys on exactly this pair.  No
-secondary indexes beyond it are declared yet: history and board queries
-(Features 08/10/13) should justify their indexes as a later migration
-once their query shapes exist.
+Uniqueness: the (definition_id, child_id, due_date, window) tuple is
+declared as a table-level ``UNIQUE`` constraint rather than a separate
+``CREATE UNIQUE INDEX`` statement.  SQLite materializes it as an
+implicit unique index (``sqlite_autoindex_quest_instances_1``) — the
+required "unique index prevents duplicate instances" — and the
+constraint form cannot be silently dropped the way a standalone index
+can, so the duplicate door stays shut.  Idempotent materialization keys
+on exactly this tuple: the twice-daily case (one definition, two
+windows, one child, one date) yields two rows and the shared case (one
+definition, three assignees, one window) yields three.  No secondary
+indexes beyond it are declared yet: history and board queries (Features
+08/10/13) should justify their indexes as a later migration once their
+query shapes exist.
 
 No ``ON DELETE`` clauses are declared on any foreign key in this
 module: children and definitions are deactivated, never deleted, so a
@@ -316,17 +322,25 @@ SCHEMA_V1_PRESENCE_OVERRIDES_DDL: list[str] = [
     """,
 ]
 
+#: Column body of ``quest_instances``, shared between the v1 DDL
+#: below and the instance-key rebuild migration, which must recreate
+#: the table in exactly this shape (D-008: the instance key is
+#: (definition_id, child_id, due_date, window) — one instance per
+#: assigned child per declared window per firing date).
+QUEST_INSTANCES_TABLE_SQL = """(
+    id INTEGER PRIMARY KEY AUTOINCREMENT,
+    definition_id INTEGER NOT NULL REFERENCES quest_definitions(id),
+    child_id INTEGER NOT NULL REFERENCES children(id),
+    window TEXT NOT NULL CHECK (window IN ('morning', 'afternoon', 'evening')),
+    due_date TEXT NOT NULL,
+    due_time TEXT,
+    generated_at TEXT NOT NULL,
+    UNIQUE (definition_id, child_id, due_date, window)
+)"""
+
 SCHEMA_V1_QUEST_INSTANCES_DDL: list[str] = [
-    """
-    CREATE TABLE IF NOT EXISTS quest_instances (
-        id INTEGER PRIMARY KEY AUTOINCREMENT,
-        definition_id INTEGER NOT NULL REFERENCES quest_definitions(id),
-        child_id INTEGER NOT NULL REFERENCES children(id),
-        due_date TEXT NOT NULL,
-        due_time TEXT,
-        generated_at TEXT NOT NULL,
-        UNIQUE (definition_id, due_date)
-    )
+    f"""
+    CREATE TABLE IF NOT EXISTS quest_instances {QUEST_INSTANCES_TABLE_SQL}
     """,
 ]