Review round 3 (Codex gpt-5.6-terra, LOW): a duplicate raw submission
reached set_admin_ids and crashed the flow with an unhandled
ValueError; the flow now rejects duplicates as invalid_admin with a
test proving the database is untouched. Suite: 905 passed.
Review rounds 1-2 (Codex gpt-5.6-terra) found two HIGHs against real HA:
- entry.context is not a durable record — the config flow now persists
the completing HA user into entry data (CONF_ADMIN_USER_IDS) and the
seed precedence becomes: options copy (the current list) first, then
the config flow's entry-data copy, then the context user as a last
resort. Config-flow tests cover the persisted user and the
context-free path; a seed-precedence test covers options > data.
- the hand-rolled closure could not serialize as an HA multi-select.
The harness now mocks homeassistant.helpers.config_validation with a
faithful multi_select, and the flow uses cv.multi_select exactly as
real HA code does. Suite: 904 passed.
- admin_allowlist.py: list_admin_ids / set_admin_ids (atomic sync of
the allowlist inside one transaction under the connection lock;
refuses empty targets so the last admin can never be removed and
the allowlist can never be emptied — fail closed) and
seed_setup_admin (seeds ONLY an empty allowlist: stored entry-data
copy first — disaster recovery for a lost database — else the HA
user who completed the config flow from the entry context;
non-empty lists are never resurrected).
- __init__.py: both setup paths (first setup and the reopened-record
path) seed after migrations; a failed seed closes the database and
re-raises through the existing error handling.
- options_flow.py: the admin picker is a multi-select of EXISTING HA
users (hass.auth.async_get_users), defaulted to the current
database allowlist, membership-checked at the schema level and
backed up on the raw-input path; empty selections are refused
(no_admins), unknown ids rejected (invalid_admin); the selection is
applied to the DB when the runtime record is reachable and always
stored on the entry as the recovery copy. Submissions without the
key (legacy raw payloads) keep the three-key behaviour untouched.
- const.CONF_ADMIN_USER_IDS; strings + translations for the field and
the two errors; conftest entries gain a context dict (HA's entry
context carries the flow user).
- Tests: 20 allowlist business tests (round-trips, refusal matrix,
seed precedence and narrowed-list persistence), 5 picker flow tests,
3 setup-seeding lifecycle tests. Suite: 901 passed.
Review round 1 (Codex gpt-5.6-terra): SQLite binds bools as integers,
so edit_child(True, ...) would silently edit child 1 and
set_child_active had the same hole (reorder already validated its
list). A shared _validate_child_id guard now runs before lookup in
edit_child, set_child_active and reorder_children's list scan, with a
coercible-id test matrix for both call shapes (True/False/1.0/"1"/
None) asserting the refused call leaves the profile untouched. Suite:
875 passed.
- ChildrenDao.reorder now requires a COMPLETE permutation of the
children table: partial lists (missing an existing child), unknown
ids and duplicates are rejected with ValueError before any write,
inside the existing transaction and connection lock (check and
write atomic). Empty list is the maximally partial reorder against
a non-empty table; against an empty table it stays a no-op. The two
DAO tests that encoded the old permissive semantics (silent no-op
on empty/unknown) are updated to the new contract.
- children.py gains reorder_children: the Feature 09-facing wrapper,
which additionally rejects non-integer ids up front (bools and
floats would silently compare against stored integer ids at the SQL
layer).
- Business tests: round-trip (active list returns new order), partial/
unknown/duplicate/non-int rejections with order-unchanged proofs,
empty-list rejection, and inactive children keeping positions in a
whole-table permutation. Suite: 866 passed.
Review round 1 (Codex gpt-5.6-terra): the DAO's int() silently coerced
strings and numerics (1, 0, 1.0) into an unintended active state.
The business layer now requires a real bool, before the existence
check's caller-intent can be lost, with a coercible-input test matrix;
the refused transition leaves the record untouched. Suite: 860 passed.
The guard test's docstring contained the phrase matching the
children/admin_users leak-guard pattern; reworded. Full suite green
before commit this time: 853 passed.
Tests for the no-data-loss deactivation contract over the existing
set_child_active business function: a child with a definition,
instance and completion event keeps all three rows byte-identical
through deactivate and reactivate, is excluded from (and restored to)
the active list, and stays in the full list. A structural guard proves
the no-delete property: no module carries DELETE targeting children
and ChildrenDao exposes no delete-shaped method. Suite: 853 passed.
Review round 3 (Codex gpt-5.6-terra) found two concurrent creates of
the same normalised name could both pass the duplicate scan before
either INSERT committed, inserting a duplicate with no warning. The
scan and INSERT now run inside the shared connection-scoped lock, so
the second create always sees the first's committed row and warns.
A gated concurrent-create test proves exactly one warning fires; a
sequential-duplicate warning assertion rode along. Suite: 851 passed.
The children/admin_users leak guard forbids children SQL outside the
DAO and its sanctioned test files; the two gated concurrency tests
probed the children table raw. They now assert via ChildrenDao.get.
Suite: 850 passed.
Review round 2 (Codex gpt-5.6-terra) found both functions returned a
readback that a concurrent writer could overwrite between the UPDATE
and the SELECT, so the returned record could be the other caller's
values. Both now run existence-check, duplicate scan, UPDATE and
readback inside the DAOs' shared connection-scoped lock. Two gated
concurrency tests prove the contract deterministically: task A paused
at its UPDATE while task B queues, each returns its own values, final
stored state is the last writer's. Suite: 850 passed.
Review round 1 (Codex gpt-5.6-terra) found: a case-only edit could
suppress the duplicate-name warning even when a DIFFERENT child
already carried the normalised name, because the edit path compared
only against the child's own old name. The scan now always runs on
the resulting state and excludes only the child being edited
(exclude_child_id), so case-only edits and stay-duplicated renames
warn like any other collision; renaming to your own name stays
warning-free when no other child holds the name. Tests: new
case-only-collision test, stay-duplicated rename assertion, and a
split self-rename test; suite at 848 passed.
Business-layer functions over ChildrenDao for creating and editing
child profiles, so the Feature 09 service gate never touches the DAO
directly.
- children.py: create_child (display name required and trimmed,
whitespace-only rejected; duplicate display name allowed but logged
as a warning, case-insensitive and whitespace-normalised on both
sides), edit_child (_UNSET sentinel for leave-alone; returns the
updated record; renaming onto a different child's name warns, own
name does not), set_child_active (the only removal path), and
list_children (display order). Explicit None for optional fields
raises instead of silently no-oping: the DAO's update deliberately
writes only provided fields and cannot express NULL, so a clear
request must not masquerade as one. UTC second-precision stamps per
the timestamp policy.
- tests/test_children_registry.py: 27 tests covering the typed
round-trips, name validation matrix, duplicate-name warning
behaviour (create and rename, including the rename-to-self
non-warning), partial edits, refusal of unsupported clears,
deactivation round-trip and list ordering.
Per IMPLEMENTATION-ORDER.md 1.5: a nullable actor_child_id column
records which panel profile was tapped; admin events stay NULL.
- schema.py: completion_events table body extracted to
COMPLETION_EVENTS_TABLE_SQL; the new column carries a FK to
children(id) and the actor-pair coherence is extended to a second
CHECK: 'user' events require actor_user_id and forbid
actor_child_id, 'panel' events require actor_child_id and forbid
actor_user_id.
- migrations.py: migration 6 rebuilds legacy-shaped completion_events
via the canonical procedure (CHECKs cannot be added to an existing
table), backfilling each legacy panel row to its own child_id
(pre-D-008 panel completions were always the tapped child) via
INSERT..SELECT — no UPDATE or DELETE path exists, so the append-only
guarantee is untouched; ids preserved; PRAGMA foreign_key_check
verified in-transaction.
- dao_instances.py: append() takes actor_child_id, validates both
actor shapes (panel requires it, user forbids it) and the tapped
child's existence under the connection lock; the record and column
list grow accordingly.
- tests: coherence CHECK matrix (5 bad shapes), unknown
actor_child_id FK, panel/user round-trips at DDL and DAO layers,
migration-6 backfill test on a frozen v2-shape seed; the
completion_events leak guard gains justified INSERT exemptions for
the migration layer (UPDATE/DELETE stay forbidden everywhere).
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.
Per IMPLEMENTATION-ORDER.md 1.4: a quest_definition_windows table
(definition_id, window, due_time) declares which day windows a
definition spans, one row per definition+window via the composite
primary key; window is CHECK-constrained to morning/afternoon/evening.
- const.py: WINDOW_MORNING/AFTERNOON/EVENING, QUEST_WINDOWS canonical
order, WINDOW_CLOCK_RANGES with the documented boundaries and the
boundary-to-later-window rule documented for Feature 07.
- schema.py: windows DDL joins SCHEMA_V1_STATEMENTS; due_time is
caller-validated strict HH:MM (DAO layer), matching the date policy.
- migrations.py: migration 4 is a plain CREATE IF NOT EXISTS statement
list (no rebuild needed for a new table).
- dao_rules.py: QuestDefinitionsDao gains upsert_window (idempotent
per window, due_time refresh in place), list_windows (const order),
remove_window; _validate_time/_validate_window helpers.
- tests: windows DDL shape/constraints (CHECK matrix, duplicate,
unknown definition), DAO round-trips, const range shapes; leak
guards extended to the new table.
Per IMPLEMENTATION-ORDER.md 1.3: a quest_definition_assignees table
(definition_id, child_id) with a composite primary key and FKs to
quest_definitions and children replaces the single child_id column on
quest_definitions.
- schema.py: definitions DDL loses child_id; new assignees table joins
SCHEMA_V1_STATEMENTS; the definitions table body is extracted to
QUEST_DEFINITIONS_TABLE_SQL so the rebuild migration cannot drift
from the canonical shape.
- migrations.py: callable migrations now own their transaction and
stamp via stamp_schema_version(database, version), because the
rebuild must bracket itself with PRAGMA foreign_keys OFF/ON. New
migration 3 creates the assignees table and, when a pre-D-008
quest_definitions still carries child_id, rebuilds the table via
SQLite's canonical procedure (create, copy, move assignees, drop,
rename), verifies PRAGMA foreign_key_check inside the transaction,
and stamps. Migration 2 converted to the new callable contract.
- dao_rules.py: QuestDefinitionsDao.create drops the child_id argument
and applies assignee_child_ids atomically; set_assignee is replaced
by add_assignee (idempotent, active-child validated under the
connection lock), remove_assignee and list_assignees; list_by_child
reads through the assignees table.
- dao_instances.py: upsert validates the child against
quest_definition_assignees instead of the definitions column.
- tests: schema shape tests for the assignees table (columns, PK, FKs,
duplicate/unknown constraints); DAO tests rewritten for the new
assignment API; migration tests for the v2 -> v3 rebuild (rows and
instances survive, child_id gone, foreign_key_check clean).
Per design/IMPLEMENTATION-ORDER.md 1.1 and decision D-007: the domain
model takes the quest vocabulary pre-release. SCHEMA_V1 now creates
quest_definitions/quest_instances directly; DAO classes, records and
error messages follow (QuestDefinitionsDao, QuestInstancesDao). The
migration runner gains migration 2, an async-callable step that detects
surviving task_* tables on dev machines and ALTER TABLE RENAMES them
inside the migration transaction, with FK clauses rewritten by SQLite.
Migration 1's content rewrite is legal because nothing was ever
shipped; dev databases stamped version 1 ride migration 2 up to 2.
Joshua King
dev_agent
orchestrator-agent
overseer
approver_agent