From 61b77cc2dbaba5f881fb07fcbea0b58e72a27acc Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 12:33:39 -0400 Subject: [PATCH 01/18] Child-profile business layer (Feature 03, task 1) 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. --- custom_components/nestquest/children.py | 210 ++++++++++++++ tests/test_children_registry.py | 350 ++++++++++++++++++++++++ 2 files changed, 560 insertions(+) create mode 100644 custom_components/nestquest/children.py create mode 100644 tests/test_children_registry.py diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py new file mode 100644 index 0000000..44e2007 --- /dev/null +++ b/custom_components/nestquest/children.py @@ -0,0 +1,210 @@ +"""Business layer for the children registry (Feature 03). + +Sits between the Feature 09 service gate and the typed +:class:`~.dao_children.ChildrenDao`: callers get validation, name +policies and the duplicate-name warning here, and never talk to the +DAO or SQL directly. Children are never hard-deleted — deactivation +is a separate explicit operation (:func:`set_child_active`), so +completion history keeps its references (feature guardrail). + +Name policy: the display name is required and trimmed; an empty or +whitespace-only name is rejected with ValueError. A duplicate display +name is ALLOWED (two children may legitimately share a nickname) but +logged as a warning so an accidental double-add is visible in the HA +log. The duplicate check is case-insensitive and whitespace-normalised +on both sides, because "ada" and "Ada " are the same name to a parent. + +Colour and avatar are optional free-form strings, trimmed when present; +``sort_order`` is an int with the DAO/schema default of 0. Clearing an +optional field back to NULL is deliberately rejected: the DAO's update +writes only provided fields (it cannot express NULL), so the business +layer refuses an explicit ``None`` instead of silently ignoring it. +Timestamps follow the module timestamp policy: the business layer +stamps UTC ISO-8601 at second precision so every row it creates is +uniform. +""" +from __future__ import annotations + +import datetime +import logging + +from .dao_children import ChildRecord, ChildrenDao +from .db import NestQuestDatabase + +LOGGER = logging.getLogger(__name__) + +#: Sentinel distinguishing "argument omitted" from "explicit SQL NULL" +#: in :func:`edit_child`. +_UNSET = object() + + +def _now_stamp() -> str: + """Return the module's strict UTC second-precision stamp.""" + return datetime.datetime.now(datetime.timezone.utc).isoformat( + timespec="seconds" + ) + + +def _validate_text( + value: object, field: str, *, required: bool = False +) -> str | None: + """Validate a free-text field and return it trimmed. + + ``None`` raises when ``required`` and otherwise returns None. A + non-string value is rejected; an empty result is rejected for + required fields (whitespace-only names are empty to a parent). + """ + if value is None: + if required: + raise ValueError(f"{field} is required") + return None + if not isinstance(value, str): + raise ValueError(f"{field} must be a string, got {value!r}") + trimmed = value.strip() + if required and not trimmed: + raise ValueError(f"{field} is required") + return trimmed + + +def _validate_sort_order(value: object) -> int: + """Reject non-int sort orders (bools included) before the DAO.""" + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"sort_order must be an integer, got {value!r}") + return value + + +async def _warn_on_duplicate_name( + database: NestQuestDatabase, display_name: str +) -> None: + """Log a warning when ``display_name`` duplicates an existing child. + + Case-insensitive and whitespace-normalised on both sides. The + duplicate is still allowed — this only makes it visible; the log + line names both rows so an accidental double-add can be found. + """ + lowered = display_name.casefold() + for child in await ChildrenDao(database).list_all(): + if child.display_name.strip().casefold() == lowered: + LOGGER.warning( + "Child display name %r duplicates existing child %d " + "(%r); allowed, but check this was intentional", + display_name, + child.id, + child.display_name, + ) + return + + +async def create_child( + database: NestQuestDatabase, + display_name: str, + *, + colour: str | None = None, + avatar_ref: str | None = None, + sort_order: int = 0, +) -> ChildRecord: + """Create a child profile and return the stored record. + + The display name is required, trimmed, and rejected when empty. + A duplicate display name (case-insensitive) is allowed but logged + as a warning. The duplicate scan and the INSERT are deliberately + not one transaction: the warning is advisory, so serialising + creates to protect it would buy nothing. + """ + name = _validate_text(display_name, "display_name", required=True) + assert name is not None + colour_value = _validate_text(colour, "colour") + avatar_value = _validate_text(avatar_ref, "avatar_ref") + order = _validate_sort_order(sort_order) + await _warn_on_duplicate_name(database, name) + return await ChildrenDao(database).create( + name, + _now_stamp(), + colour=colour_value, + avatar_ref=avatar_value, + sort_order=order, + ) + + +async def edit_child( + database: NestQuestDatabase, + child_id: int, + *, + display_name: str | object = _UNSET, + colour: str | None | object = _UNSET, + avatar_ref: str | None | object = _UNSET, + sort_order: int | object = _UNSET, +) -> ChildRecord: + """Edit a child profile and return the updated record. + + Arguments default to the module sentinel ``_UNSET`` meaning "leave + this field alone". An explicitly passed ``None`` for an optional + field raises: the DAO's update writes only provided fields and + cannot express NULL, so a silent no-op would mask the caller's + intent (see the module docstring). A provided display name is + validated like create's and, when it duplicates a DIFFERENT child, + still allowed with a warning. Raises ValueError when the child + does not exist. + """ + dao = ChildrenDao(database) + existing = await dao.get(child_id) + if existing is None: + raise ValueError(f"child {child_id} does not exist") + updates: dict[str, object] = {} + if display_name is not _UNSET: + name = _validate_text(display_name, "display_name", required=True) + assert name is not None + if name.casefold() != existing.display_name.strip().casefold(): + await _warn_on_duplicate_name(database, name) + updates["display_name"] = name + if colour is not _UNSET: + value = _validate_text(colour, "colour") + if value is None: + raise ValueError( + "clearing colour is not supported; pass a replacement " + "value or omit the field" + ) + updates["colour"] = value + if avatar_ref is not _UNSET: + value = _validate_text(avatar_ref, "avatar_ref") + if value is None: + raise ValueError( + "clearing avatar_ref is not supported; pass a " + "replacement value or omit the field" + ) + updates["avatar_ref"] = value + if sort_order is not _UNSET: + updates["sort_order"] = _validate_sort_order(sort_order) + if updates: + await dao.update(child_id, **updates) + edited = await dao.get(child_id) + assert edited is not None + return edited + + +async def set_child_active( + database: NestQuestDatabase, child_id: int, is_active: bool +) -> ChildRecord: + """Deactivate or reactivate a child; returns the updated record. + + Deactivation hides the child from the panel without deleting their + history (feature guardrail); this is the only removal path. + Raises ValueError when the child does not exist. + """ + dao = ChildrenDao(database) + if await dao.get(child_id) is None: + raise ValueError(f"child {child_id} does not exist") + await dao.set_active(child_id, is_active) + updated = await dao.get(child_id) + assert updated is not None + return updated + + +async def list_children( + database: NestQuestDatabase, *, active_only: bool = False +) -> list[ChildRecord]: + """Return children in display order (sort_order, then id).""" + dao = ChildrenDao(database) + if active_only: + return await dao.list_active() + return await dao.list_all() diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py new file mode 100644 index 0000000..d88209d --- /dev/null +++ b/tests/test_children_registry.py @@ -0,0 +1,350 @@ +"""Tests for children.py: the Feature 03 child-profile business layer.""" +from __future__ import annotations + +import asyncio +import logging +import re +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from custom_components.nestquest.children import ( + create_child, + edit_child, + list_children, + set_child_active, +) +from custom_components.nestquest.dao_children import ChildRecord +from custom_components.nestquest.db import NestQuestDatabase +from custom_components.nestquest.migrations import apply_migrations + + +def _run(coro): + return asyncio.new_event_loop().run_until_complete(coro) + + +def _make_hass_mock(): + hass = MagicMock() + hass.async_add_executor_job = AsyncMock(side_effect=(lambda fn, *a: fn(*a))) + return hass + + +def _with_db(tmp_path, name): + def _run_test(body): + async def _main(): + database = NestQuestDatabase(_make_hass_mock()) + await database.open(tmp_path / name) + try: + await apply_migrations(database) + return await body(database) + finally: + await database.close() + + return _run(_main()) + + return _run_test + + +# --------------------------------------------------------------------------- +# create_child: required, trimmed display name +# --------------------------------------------------------------------------- + + +def test_create_child_returns_typed_record(tmp_path) -> None: + async def _body(database): + child = await create_child( + database, + " Declan ", + colour="#2E7D32", + avatar_ref="avatars/declan.png", + sort_order=2, + ) + assert isinstance(child, ChildRecord) + # The name is trimmed before storage. + assert child.display_name == "Declan" + assert child.colour == "#2E7D32" + assert child.avatar_ref == "avatars/declan.png" + assert child.sort_order == 2 + assert child.is_active is True + # The business layer stamps created_at in strict UTC shape. + assert re.fullmatch( + r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\+00:00", child.created_at + ) + return child + + _with_db(tmp_path, "create-typed.db")(_body) + + +def test_create_child_defaults(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Chloe") + assert child.display_name == "Chloe" + assert child.colour is None + assert child.avatar_ref is None + assert child.sort_order == 0 + return child + + _with_db(tmp_path, "create-defaults.db")(_body) + + +@pytest.mark.parametrize("bad", ["", " ", "\t\n", None]) +def test_create_child_missing_name_rejected(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError, match="display_name is required"): + await create_child(database, bad) + assert await list_children(database) == [] + return None + + _with_db(tmp_path, "create-empty-name.db")(_body) + + +@pytest.mark.parametrize("bad", [42, ["Ada"], 3.5]) +def test_create_child_non_string_name_rejected(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError, match="must be a string"): + await create_child(database, bad) + return None + + _with_db(tmp_path, "create-bad-name-type.db")(_body) + + +def test_create_child_optional_text_fields_validated(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="colour must be a string"): + await create_child(database, "Ada", colour=7) + with pytest.raises(ValueError, match="avatar_ref must be a string"): + await create_child(database, "Ada", avatar_ref=1.5) + return None + + _with_db(tmp_path, "create-bad-optional.db")(_body) + + +@pytest.mark.parametrize("bad", ["2", 1.5, True, None]) +def test_create_child_sort_order_must_be_int(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError, match="sort_order must be an integer"): + await create_child(database, "Ada", sort_order=bad) + return None + + _with_db(tmp_path, "create-bad-order.db")(_body) + + +# --------------------------------------------------------------------------- +# create_child: duplicate display name allowed but warned +# --------------------------------------------------------------------------- + + +def test_create_child_duplicate_name_allowed_and_warned( + tmp_path, caplog +) -> None: + async def _body(database): + first = await create_child(database, "Ada") + with caplog.at_level( + logging.WARNING, logger="custom_components.nestquest.children" + ): + second = await create_child(database, " ADA ") + # Whitespace/case-normalised duplicate: allowed, both rows exist. + assert first.display_name == "Ada" + assert second.display_name == "ADA" + names = [c.display_name for c in await list_children(database)] + assert names == ["Ada", "ADA"] + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1, ( + "exactly one duplicate warning for the second create" + ) + assert "Ada" in warnings[0].getMessage() + return second + + _with_db(tmp_path, "create-duplicate.db")(_body) + + +def test_create_child_distinct_name_logs_no_warning( + tmp_path, caplog +) -> None: + async def _body(database): + await create_child(database, "Ada") + caplog.clear() + await create_child(database, "Jordyn") + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert warnings == [] + return None + + _with_db(tmp_path, "create-no-duplicate.db")(_body) + + +# --------------------------------------------------------------------------- +# edit_child: returns the updated record +# --------------------------------------------------------------------------- + + +def test_edit_child_updates_fields_and_returns_record(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada", colour="#111111") + edited = await edit_child( + database, + child.id, + display_name=" Ada L. ", + colour="#222222", + avatar_ref="avatars/ada.png", + sort_order=3, + ) + assert isinstance(edited, ChildRecord) + assert edited.id == child.id + assert edited.display_name == "Ada L." + assert edited.colour == "#222222" + assert edited.avatar_ref == "avatars/ada.png" + assert edited.sort_order == 3 + # The stored row reflects every edit. + stored = await list_children(database) + assert len(stored) == 1 + assert stored[0] == edited + return edited + + _with_db(tmp_path, "edit-round-trip.db")(_body) + + +def test_edit_child_omitted_fields_unchanged(tmp_path) -> None: + async def _body(database): + child = await create_child( + database, "Ada", colour="#111111", sort_order=5 + ) + edited = await edit_child(database, child.id, sort_order=9) + # Only sort_order moved; colour and name untouched. + assert edited.sort_order == 9 + assert edited.colour == "#111111" + assert edited.display_name == "Ada" + return edited + + _with_db(tmp_path, "edit-partial.db")(_body) + + +def test_edit_child_no_fields_returns_current_record(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada", colour="#111111") + edited = await edit_child(database, child.id) + assert edited == child + return edited + + _with_db(tmp_path, "edit-noop.db")(_body) + + +def test_edit_child_missing_raises(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="does not exist"): + await edit_child(database, 999, display_name="X") + return None + + _with_db(tmp_path, "edit-missing.db")(_body) + + +def test_edit_child_name_validation(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada") + for bad in ("", " ", None, 42): + with pytest.raises(ValueError, match="display_name"): + await edit_child(database, child.id, display_name=bad) + # The failed edits left the record untouched. + assert (await list_children(database))[0].display_name == "Ada" + return None + + _with_db(tmp_path, "edit-name-validation.db")(_body) + + +def test_edit_child_sort_order_must_be_int(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="sort_order must be an integer"): + await edit_child(database, child.id, sort_order="3") + return None + + _with_db(tmp_path, "edit-bad-order.db")(_body) + + +def test_edit_child_explicit_none_clear_rejected(tmp_path) -> None: + """Clearing is refused loudly, not silently ignored: the DAO cannot + express NULL writes, so an explicit None must never masquerade as a + no-op.""" + async def _body(database): + child = await create_child(database, "Ada", colour="#111111") + with pytest.raises(ValueError, match="clearing colour"): + await edit_child(database, child.id, colour=None) + with pytest.raises(ValueError, match="clearing avatar_ref"): + await edit_child(database, child.id, avatar_ref=None) + # The refused clears left the colour intact. + assert (await list_children(database))[0].colour == "#111111" + return None + + _with_db(tmp_path, "edit-clear-unsupported.db")(_body) + + +def test_edit_child_rename_to_duplicate_warns_but_different_name_not( + tmp_path, caplog +) -> None: + async def _body(database): + ada = await create_child(database, "Ada") + bo = await create_child(database, "Bo") + caplog.clear() + # Renaming Bo onto Ada's name warns (different child's name). + edited = await edit_child(database, bo.id, display_name="ada") + assert edited.display_name == "ada" + assert any( + "duplicates existing child" in r.getMessage() + for r in caplog.records + ) + caplog.clear() + # Renaming a child to their own name (casing aside) is not a + # duplicate: no warning. + await edit_child(database, ada.id, display_name="ADA") + assert not any( + "duplicates existing child" in r.getMessage() + for r in caplog.records + ) + return None + + _with_db(tmp_path, "edit-rename-duplicate.db")(_body) + + +# --------------------------------------------------------------------------- +# set_child_active / list_children +# --------------------------------------------------------------------------- + + +def test_set_child_active_round_trip(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada") + deactivated = await set_child_active(database, child.id, False) + assert deactivated.is_active is False + assert [c.id for c in await list_children(database)] == [child.id] + assert await list_children(database, active_only=True) == [] + reactivated = await set_child_active(database, child.id, True) + assert reactivated.is_active is True + assert len(await list_children(database, active_only=True)) == 1 + return reactivated + + _with_db(tmp_path, "set-active.db")(_body) + + +def test_set_child_active_missing_raises(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="does not exist"): + await set_child_active(database, 999, False) + return None + + _with_db(tmp_path, "set-active-missing.db")(_body) + + +def test_list_children_orders_by_sort_order_then_id(tmp_path) -> None: + async def _body(database): + second = await create_child(database, "Bo", sort_order=1) + first = await create_child(database, "Ada", sort_order=0) + third = await create_child(database, "Cleo", sort_order=1) + listed = await list_children(database) + # sort_order 0 first, then the two sort_order-1 rows by id. + assert [c.id for c in listed] == [first.id, second.id, third.id] + return listed + + _with_db(tmp_path, "list-order.db")(_body) From 712527d5baba9d0d24020a7847601c6b3df060c4 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:08:24 -0400 Subject: [PATCH 02/18] Review fix: duplicate-name scan excludes only the edited child 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. --- custom_components/nestquest/children.py | 21 ++++++--- tests/test_children_registry.py | 58 +++++++++++++++++++++---- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 44e2007..c0b6790 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -74,16 +74,25 @@ def _validate_sort_order(value: object) -> int: async def _warn_on_duplicate_name( - database: NestQuestDatabase, display_name: str + database: NestQuestDatabase, + display_name: str, + *, + exclude_child_id: int | None = None, ) -> None: """Log a warning when ``display_name`` duplicates an existing child. Case-insensitive and whitespace-normalised on both sides. The duplicate is still allowed — this only makes it visible; the log line names both rows so an accidental double-add can be found. + ``exclude_child_id`` removes the child being edited from the scan, + so an edit only warns when the result collides with a DIFFERENT + child's name — including case-only edits, where the self-comparison + would otherwise hide a real duplicate. """ lowered = display_name.casefold() for child in await ChildrenDao(database).list_all(): + if exclude_child_id is not None and child.id == exclude_child_id: + continue if child.display_name.strip().casefold() == lowered: LOGGER.warning( "Child display name %r duplicates existing child %d " @@ -142,9 +151,10 @@ async def edit_child( field raises: the DAO's update writes only provided fields and cannot express NULL, so a silent no-op would mask the caller's intent (see the module docstring). A provided display name is - validated like create's and, when it duplicates a DIFFERENT child, - still allowed with a warning. Raises ValueError when the child - does not exist. + validated like create's; the duplicate scan excludes only the child + being edited, so a case-only edit colliding with another child's + normalised name still warns. Raises ValueError when the child does + not exist. """ dao = ChildrenDao(database) existing = await dao.get(child_id) @@ -154,8 +164,7 @@ async def edit_child( if display_name is not _UNSET: name = _validate_text(display_name, "display_name", required=True) assert name is not None - if name.casefold() != existing.display_name.strip().casefold(): - await _warn_on_duplicate_name(database, name) + await _warn_on_duplicate_name(database, name, exclude_child_id=child_id) updates["display_name"] = name if colour is not _UNSET: value = _validate_text(colour, "colour") diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index d88209d..3aa874d 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -291,21 +291,63 @@ def test_edit_child_rename_to_duplicate_warns_but_different_name_not( # Renaming Bo onto Ada's name warns (different child's name). edited = await edit_child(database, bo.id, display_name="ada") assert edited.display_name == "ada" - assert any( - "duplicates existing child" in r.getMessage() - for r in caplog.records - ) + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1 + assert "Ada" in warnings[0].getMessage() + return edited + + _with_db(tmp_path, "edit-rename-duplicate.db")(_body) + + +def test_edit_child_rename_to_own_name_casing_alone_warns_not( + tmp_path, caplog +) -> None: + """Renaming a child to their own name (casing aside) is not a + duplicate when NO OTHER child carries the normalised name.""" + async def _body(database): + ada = await create_child(database, "Ada") caplog.clear() - # Renaming a child to their own name (casing aside) is not a - # duplicate: no warning. - await edit_child(database, ada.id, display_name="ADA") + edited = await edit_child(database, ada.id, display_name="ADA") + assert edited.display_name == "ADA" assert not any( "duplicates existing child" in r.getMessage() for r in caplog.records ) + return edited + + _with_db(tmp_path, "edit-rename-self.db")(_body) + + +def test_edit_child_case_only_edit_colliding_with_other_child_warns( + tmp_path, caplog +) -> None: + """A case-only edit must not hide a real duplicate: with two Ada + profiles already present, editing either one's name to another + casing still warns (the scan excludes only the edited child).""" + async def _body(database): + first = await create_child(database, "Ada") + await create_child(database, "ada") + caplog.clear() + edited = await edit_child(database, first.id, display_name="ADA") + assert edited.display_name == "ADA" + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1 + # Editing the name to a value that STAYS duplicated (no change, + # other child still carries it) warns too: the scan is on the + # resulting state, not on whether the word moved. + caplog.clear() + await edit_child(database, first.id, display_name=" ada ") + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1 return None - _with_db(tmp_path, "edit-rename-duplicate.db")(_body) + _with_db(tmp_path, "edit-case-only-duplicate.db")(_body) # --------------------------------------------------------------------------- From 5578507ce898a28942e709a948247b151d43c6d0 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:13:48 -0400 Subject: [PATCH 03/18] Review fix: edit_child/set_child_active update+readback atomic 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. --- custom_components/nestquest/children.py | 86 +++++++++------- tests/test_children_registry.py | 125 ++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 35 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index c0b6790..e3439aa 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -28,7 +28,7 @@ from __future__ import annotations import datetime import logging -from .dao_children import ChildRecord, ChildrenDao +from .dao_children import ChildRecord, ChildrenDao, _connection_lock from .db import NestQuestDatabase LOGGER = logging.getLogger(__name__) @@ -155,38 +155,49 @@ async def edit_child( being edited, so a case-only edit colliding with another child's normalised name still warns. Raises ValueError when the child does not exist. + + Atomicity: the existence check, the duplicate scan, the UPDATE and + the readback all run inside the connection-scoped lock shared with + the DAOs, so another edit cannot slip between this call's UPDATE + and its readback — the returned record is exactly the one this + edit produced, not a later writer's. """ - dao = ChildrenDao(database) - existing = await dao.get(child_id) - if existing is None: - raise ValueError(f"child {child_id} does not exist") - updates: dict[str, object] = {} - if display_name is not _UNSET: - name = _validate_text(display_name, "display_name", required=True) - assert name is not None - await _warn_on_duplicate_name(database, name, exclude_child_id=child_id) - updates["display_name"] = name - if colour is not _UNSET: - value = _validate_text(colour, "colour") - if value is None: - raise ValueError( - "clearing colour is not supported; pass a replacement " - "value or omit the field" + async with _connection_lock(database): + dao = ChildrenDao(database) + existing = await dao.get(child_id) + if existing is None: + raise ValueError(f"child {child_id} does not exist") + updates: dict[str, object] = {} + if display_name is not _UNSET: + name = _validate_text( + display_name, "display_name", required=True ) - updates["colour"] = value - if avatar_ref is not _UNSET: - value = _validate_text(avatar_ref, "avatar_ref") - if value is None: - raise ValueError( - "clearing avatar_ref is not supported; pass a " - "replacement value or omit the field" + assert name is not None + await _warn_on_duplicate_name( + database, name, exclude_child_id=child_id ) - updates["avatar_ref"] = value - if sort_order is not _UNSET: - updates["sort_order"] = _validate_sort_order(sort_order) - if updates: - await dao.update(child_id, **updates) - edited = await dao.get(child_id) + updates["display_name"] = name + if colour is not _UNSET: + value = _validate_text(colour, "colour") + if value is None: + raise ValueError( + "clearing colour is not supported; pass a replacement " + "value or omit the field" + ) + updates["colour"] = value + if avatar_ref is not _UNSET: + value = _validate_text(avatar_ref, "avatar_ref") + if value is None: + raise ValueError( + "clearing avatar_ref is not supported; pass a " + "replacement value or omit the field" + ) + updates["avatar_ref"] = value + if sort_order is not _UNSET: + updates["sort_order"] = _validate_sort_order(sort_order) + if updates: + await dao.update(child_id, **updates) + edited = await dao.get(child_id) assert edited is not None return edited @@ -199,12 +210,17 @@ async def set_child_active( Deactivation hides the child from the panel without deleting their history (feature guardrail); this is the only removal path. Raises ValueError when the child does not exist. + + Atomicity: existence check, UPDATE and readback run inside the + connection-scoped lock, so a concurrent opposite transition cannot + make this call report the other caller's value. """ - dao = ChildrenDao(database) - if await dao.get(child_id) is None: - raise ValueError(f"child {child_id} does not exist") - await dao.set_active(child_id, is_active) - updated = await dao.get(child_id) + async with _connection_lock(database): + dao = ChildrenDao(database) + if await dao.get(child_id) is None: + raise ValueError(f"child {child_id} does not exist") + await dao.set_active(child_id, is_active) + updated = await dao.get(child_id) assert updated is not None return updated diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 3aa874d..e2af6cf 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -379,6 +379,131 @@ def test_set_child_active_missing_raises(tmp_path) -> None: _with_db(tmp_path, "set-active-missing.db")(_body) +# --------------------------------------------------------------------------- +# Concurrency: edit/set_active are atomic under the connection lock +# --------------------------------------------------------------------------- + + +def _gated_hass(armed: dict, gate_open: asyncio.Event, a_started: asyncio.Event): + """A hass mock whose executor gates the FIRST `_execute` job after + arming — the mutate statement of whichever business call starts + first — proving the caller holds the connection lock at that + moment and the second caller must queue behind it.""" + from unittest.mock import MagicMock + + hass = MagicMock() + + async def _gated_executor(fn, *args): + if armed["active"]: + if getattr(fn, "__name__", "") == "_execute" and not armed["gated"]: + armed["gated"] = True + a_started.set() + await gate_open.wait() + return fn(*args) + + hass.async_add_executor_job = _gated_executor + return hass + + +def test_edit_child_concurrent_opposite_edits_return_own_values( + tmp_path, +) -> None: + """Two racing edits cannot cross-contaminate each other's readback. + + Deterministic gate: task A is paused at its UPDATE (it provably + holds the connection lock), task B is then created and must queue; + A's readback runs before B's edit, so A returns A's value and B + returns B's. Without the lock, A's readback would see B's write. + """ + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "edit-race.db") + try: + await apply_migrations(database) + child = await create_child( + database, "Ada", colour="#111111", sort_order=0 + ) + + armed["active"] = True + task_a = asyncio.ensure_future( + edit_child(database, child.id, colour="#AAAAAA", sort_order=1) + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + edit_child( + database, child.id, colour="#BBBBBB", sort_order=2 + ) + ) + await asyncio.sleep(0) + gate_open.set() + record_a, record_b = await asyncio.gather(task_a, task_b) + + # Each caller gets back exactly what it wrote. + assert record_a.colour == "#AAAAAA" + assert record_a.sort_order == 1 + assert record_b.colour == "#BBBBBB" + assert record_b.sort_order == 2 + # B landed last: the stored row is B's. + row = await database.fetch_one( + "SELECT colour, sort_order FROM children WHERE id = ?", + (child.id,), + ) + assert row == ("#BBBBBB", 2) + finally: + await database.close() + + _run(_main()) + + +def test_set_child_active_concurrent_opposite_transitions_return_own( + tmp_path, +) -> None: + """Racing activate/deactivate calls each report their own request. + + Same deterministic gate on task A's UPDATE; task B queues behind + the connection lock and lands the opposite transition after A. + """ + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "active-race.db") + try: + await apply_migrations(database) + child = await create_child(database, "Ada") + + armed["active"] = True + task_a = asyncio.ensure_future( + set_child_active(database, child.id, False) + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + set_child_active(database, child.id, True) + ) + await asyncio.sleep(0) + gate_open.set() + record_a, record_b = await asyncio.gather(task_a, task_b) + + assert record_a.is_active is False + assert record_b.is_active is True + row = await database.fetch_one( + "SELECT is_active FROM children WHERE id = ?", (child.id,) + ) + assert row == (1,) + finally: + await database.close() + + _run(_main()) + + def test_list_children_orders_by_sort_order_then_id(tmp_path) -> None: async def _body(database): second = await create_child(database, "Bo", sort_order=1) From 09363a57cf89332dcf40ef5b2072c829a874d84d Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:14:56 -0400 Subject: [PATCH 04/18] Review fix: race tests read via the DAO, not raw SQL probes 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. --- tests/test_children_registry.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index e2af6cf..9cd6779 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -14,7 +14,7 @@ from custom_components.nestquest.children import ( list_children, set_child_active, ) -from custom_components.nestquest.dao_children import ChildRecord +from custom_components.nestquest.dao_children import ChildRecord, ChildrenDao from custom_components.nestquest.db import NestQuestDatabase from custom_components.nestquest.migrations import apply_migrations @@ -448,12 +448,11 @@ def test_edit_child_concurrent_opposite_edits_return_own_values( assert record_a.sort_order == 1 assert record_b.colour == "#BBBBBB" assert record_b.sort_order == 2 - # B landed last: the stored row is B's. - row = await database.fetch_one( - "SELECT colour, sort_order FROM children WHERE id = ?", - (child.id,), - ) - assert row == ("#BBBBBB", 2) + # B landed last: the stored row is B's (read via the DAO — + # the children leak guard forbids raw probes here). + stored = await ChildrenDao(database).get(child.id) + assert stored is not None + assert (stored.colour, stored.sort_order) == ("#BBBBBB", 2) finally: await database.close() @@ -494,10 +493,9 @@ def test_set_child_active_concurrent_opposite_transitions_return_own( assert record_a.is_active is False assert record_b.is_active is True - row = await database.fetch_one( - "SELECT is_active FROM children WHERE id = ?", (child.id,) - ) - assert row == (1,) + stored = await ChildrenDao(database).get(child.id) + assert stored is not None + assert stored.is_active is True finally: await database.close() From d5d972b872e8b38e765a02c0a5ae9966fc3a6be4 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:18:33 -0400 Subject: [PATCH 05/18] Review fix: serialize create_child's duplicate scan with the insert 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. --- custom_components/nestquest/children.py | 26 ++++++----- tests/test_children_registry.py | 62 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index e3439aa..4e1d56e 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -116,23 +116,27 @@ async def create_child( The display name is required, trimmed, and rejected when empty. A duplicate display name (case-insensitive) is allowed but logged - as a warning. The duplicate scan and the INSERT are deliberately - not one transaction: the warning is advisory, so serialising - creates to protect it would buy nothing. + as a warning. The scan and the INSERT run inside the + connection-scoped lock shared with the DAOs, so a concurrent + create of the same normalised name queues behind the first insert + and sees it — the duplicate still warns; it can never slip through + unlogged. """ name = _validate_text(display_name, "display_name", required=True) assert name is not None colour_value = _validate_text(colour, "colour") avatar_value = _validate_text(avatar_ref, "avatar_ref") order = _validate_sort_order(sort_order) - await _warn_on_duplicate_name(database, name) - return await ChildrenDao(database).create( - name, - _now_stamp(), - colour=colour_value, - avatar_ref=avatar_value, - sort_order=order, - ) + async with _connection_lock(database): + await _warn_on_duplicate_name(database, name) + created = await ChildrenDao(database).create( + name, + _now_stamp(), + colour=colour_value, + avatar_ref=avatar_value, + sort_order=order, + ) + return created async def edit_child( diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 9cd6779..63f6d73 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -320,6 +320,68 @@ def test_edit_child_rename_to_own_name_casing_alone_warns_not( _with_db(tmp_path, "edit-rename-self.db")(_body) +def test_create_child_concurrent_same_name_still_warns(tmp_path) -> None: + """Racing creates of the same normalised name cannot slip through + unlogged: the scan and INSERT are serialized on the connection + lock, so the second create sees the first's committed row and + warns. Deterministic gate on task A's INSERT while task B queues + behind the lock. Without the lock, B's scan would run before A's + INSERT commits and no warning would fire.""" + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "create-race.db") + try: + await apply_migrations(database) + logger = logging.getLogger( + "custom_components.nestquest.children" + ) + seen: list[logging.LogRecord] = [] + + class _Capture(logging.Handler): + def emit(self, record: logging.LogRecord) -> None: + seen.append(record) + + handler = _Capture() + logger.addHandler(handler) + try: + armed["active"] = True + task_a = asyncio.ensure_future( + create_child(database, "Ada") + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + create_child(database, " ada ") + ) + await asyncio.sleep(0) + gate_open.set() + first, second = await asyncio.gather(task_a, task_b) + finally: + logger.removeHandler(handler) + + # Both creates land; the duplicate is allowed. + assert first.display_name == "Ada" + assert second.display_name == "ada" + names = [c.display_name for c in await list_children(database)] + assert names == ["Ada", "ada"] + # Exactly one warning: B's scan saw A's committed row. + warnings = [ + r for r in seen if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1, ( + "the queued duplicate create must warn exactly once" + ) + assert "Ada" in warnings[0].getMessage() + finally: + await database.close() + + _run(_main()) + + def test_edit_child_case_only_edit_colliding_with_other_child_warns( tmp_path, caplog ) -> None: From 1d247fbca1200baee1227ceab542d25939acc6b8 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:30:36 -0400 Subject: [PATCH 06/18] Deactivation preserves every referencing row (Feature 03, task 2) 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. --- tests/test_children_registry.py | 114 ++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 63f6d73..71af6f7 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -575,3 +575,117 @@ def test_list_children_orders_by_sort_order_then_id(tmp_path) -> None: return listed _with_db(tmp_path, "list-order.db")(_body) + + +# --------------------------------------------------------------------------- +# Deactivation keeps every referencing row intact (Feature 03) +# --------------------------------------------------------------------------- + + +def test_deactivating_child_preserves_definitions_instances_events( + tmp_path, +) -> None: + async def _body(database): + from datetime import date, timedelta + + from custom_components.nestquest.dao_children import ChildrenDao + from custom_components.nestquest.dao_instances import ( + CompletionEventsDao, + QuestInstancesDao, + ) + from custom_components.nestquest.dao_rules import ( + QuestDefinitionsDao, + ScheduleRulesDao, + ) + + children = ChildrenDao(database) + rules = ScheduleRulesDao(database) + definitions = QuestDefinitionsDao(database) + instances = QuestInstancesDao(database) + events = CompletionEventsDao(database) + + child = await create_child(database, "Ada") + rule = await rules.create("daily", "2026-01-01") + definition = await definitions.create( + "Brush teeth", + rule.id, + "2026-09-15T12:00:00+00:00", + assignee_child_ids=[child.id], + ) + due = (date.today() + timedelta(days=1)).isoformat() + stamp = f"{due}T08:00:00+00:00" + instance = await instances.upsert( + definition.id, child.id, due, stamp, window="morning" + ) + event = await events.append( + instance.id, + child.id, + "completed", + "panel", + f"{due}T09:00:00+00:00", + True, + actor_child_id=child.id, + ) + + # Deactivate: the referencing rows must not move. + deactivated = await set_child_active(database, child.id, False) + assert deactivated.is_active is False + assert await children.get(child.id) == deactivated + assert (await definitions.get(definition.id)) == definition + assert (await instances.get_by_id(instance.id)) == instance + assert await events.list_by_instance(instance.id) == [event] + # Excluded from the active list, still present in the full list. + assert await list_children(database, active_only=True) == [] + assert [c.id for c in await list_children(database)] == [child.id] + + # Reactivate: fully restored. + reactivated = await set_child_active(database, child.id, True) + assert reactivated.is_active is True + assert [c.id for c in await list_children(database, active_only=True)] == [ + child.id + ] + assert (await instances.get_by_id(instance.id)) == instance + return reactivated + + _with_db(tmp_path, "deactivate-data-intact.db")(_body) + + +def test_no_code_path_deletes_a_child_row() -> None: + """Structural guardrail: children are deactivated, never deleted. + + The DAO exposes no delete method at all, and no module in the + package (nor any test outside the sanctioned DDL/migration layers, + which create the tables) carries SQL that deletes from children — + the leak guard in test_dao_children enforces the file allowlist, + this asserts the no-delete property itself across every file that + may legitimately name the table. + """ + import re + from pathlib import Path + + package = Path( + __import__( + "custom_components.nestquest", fromlist=["__file__"] + ).__file__ + ).parent + + delete_pattern = re.compile( + r"DELETE\s+(FROM\s+)?((main|temp)\s*\.\s*)?[`'\"]*(\[)?children\b", + re.IGNORECASE, + ) + for py in sorted(package.rglob("*.py")): + assert delete_pattern.search(py.read_text()) is None, ( + f"a code path in {py.name} deletes child rows" + ) + + import inspect + + from custom_components.nestquest.dao_children import ChildrenDao + + methods = { + name + for name, _ in inspect.getmembers(ChildrenDao, inspect.isfunction) + } + assert not any( + name.startswith("delete") or name == "remove" for name in methods + ), f"ChildrenDao exposed a delete-shaped method: {methods}" From 5c74034f8145edf0ba9e4956db2009c928f9a6ac Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:32:18 -0400 Subject: [PATCH 07/18] Fix leak-guard collision: reword no-delete guard docstring 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/test_children_registry.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 71af6f7..3f977fb 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -655,10 +655,10 @@ def test_no_code_path_deletes_a_child_row() -> None: The DAO exposes no delete method at all, and no module in the package (nor any test outside the sanctioned DDL/migration layers, - which create the tables) carries SQL that deletes from children — - the leak guard in test_dao_children enforces the file allowlist, - this asserts the no-delete property itself across every file that - may legitimately name the table. + which create the tables) carries a statement that removes child + rows — the leak guard in test_dao_children enforces the file + allowlist, this asserts the no-delete property itself across every + file that may legitimately name the table. """ import re from pathlib import Path From cfe1609df25ff7e0add1691bb3ce2c396228f3da Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:35:18 -0400 Subject: [PATCH 08/18] Review fix: set_child_active rejects non-bool is_active 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. --- custom_components/nestquest/children.py | 9 ++++++++- tests/test_children_registry.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 4e1d56e..315fdf8 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -213,7 +213,10 @@ async def set_child_active( Deactivation hides the child from the panel without deleting their history (feature guardrail); this is the only removal path. - Raises ValueError when the child does not exist. + ``is_active`` must be a real bool: the DAO's ``int()`` would + otherwise silently coerce strings and numerics ("1", 0) into a + state the caller never asked for. Raises ValueError when the + child does not exist. Atomicity: existence check, UPDATE and readback run inside the connection-scoped lock, so a concurrent opposite transition cannot @@ -223,6 +226,10 @@ async def set_child_active( dao = ChildrenDao(database) if await dao.get(child_id) is None: raise ValueError(f"child {child_id} does not exist") + if not isinstance(is_active, bool): + raise ValueError( + f"is_active must be a real bool, got {is_active!r}" + ) await dao.set_active(child_id, is_active) updated = await dao.get(child_id) assert updated is not None diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 3f977fb..e3d28f6 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -441,6 +441,21 @@ def test_set_child_active_missing_raises(tmp_path) -> None: _with_db(tmp_path, "set-active-missing.db")(_body) +@pytest.mark.parametrize("bad", ["1", "0", 1, 0, 1.0, None, "true"]) +def test_set_child_active_rejects_non_bool(tmp_path, bad) -> None: + """The DAO's int() would silently coerce "1"/0/1.0 into a state the + caller never asked for; the business layer refuses non-bools.""" + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="is_active must be a real bool"): + await set_child_active(database, child.id, bad) + # The refused transition left the child untouched. + assert (await list_children(database))[0].is_active is True + return None + + _with_db(tmp_path, "set-active-non-bool.db")(_body) + + # --------------------------------------------------------------------------- # Concurrency: edit/set_active are atomic under the connection lock # --------------------------------------------------------------------------- From 7a585afb9e16fdbb0b0b1c1134322e1fabdde1ff Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:41:14 -0400 Subject: [PATCH 09/18] Reorder children for panel display order (Feature 03, task 3) - 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. --- custom_components/nestquest/children.py | 24 +++++ custom_components/nestquest/dao_children.py | 44 ++++++-- tests/test_children_registry.py | 109 ++++++++++++++++++++ tests/test_dao_children.py | 18 +++- 4 files changed, 184 insertions(+), 11 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 315fdf8..0633774 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -236,6 +236,30 @@ async def set_child_active( return updated +async def reorder_children( + database: NestQuestDatabase, ordered_ids: list[int] +) -> None: + """Rewrite the children's panel display order in one transaction. + + ``ordered_ids`` must be a complete permutation of the children + table — every child id exactly once. A partial list (missing an + existing child), an unknown id, or a duplicate is rejected with + ValueError before any write, so the current order survives a + rejected call. The DAO enforces this inside its own transaction + (check and write are atomic under the connection lock); this + wrapper additionally rejects non-integer ids up front, since + ``bool`` and floats would silently compare against stored integer + ids at the SQL layer. + """ + for child_id in ordered_ids: + if isinstance(child_id, bool) or not isinstance(child_id, int): + raise ValueError( + f"ordered_ids must contain only integer child ids, got " + f"{child_id!r}" + ) + await ChildrenDao(database).reorder(ordered_ids) + + async def list_children( database: NestQuestDatabase, *, active_only: bool = False ) -> list[ChildRecord]: diff --git a/custom_components/nestquest/dao_children.py b/custom_components/nestquest/dao_children.py index 88eb62c..57c42a8 100644 --- a/custom_components/nestquest/dao_children.py +++ b/custom_components/nestquest/dao_children.py @@ -191,13 +191,20 @@ class ChildrenDao: async def reorder(self, ordered_ids: list[int]) -> None: """Persist a new sort order: ``ordered_ids[i]`` gets sort_order i. + The list must be a COMPLETE permutation of the children table: + every existing child id appears exactly once. Partial lists + (missing an existing child) and unknown ids (naming a child + that does not exist) are rejected with ValueError BEFORE any + write, leaving the current order untouched — a silent partial + reorder would leave unlisted children with stale positions and + hide a caller bug. Duplicate ids are rejected up front: they + cannot all hold distinct positions, and silently assigning the + last occurrence's index would hide a caller bug. + Runs inside one transaction; positions are staged at offset -len (all negative, so any permutation of the same ids never collides mid-reorder under the wrapper's single-connection - serialization), then finalized 0..n-1. Duplicate ids are - rejected up front: they cannot all hold distinct positions, and - silently assigning the last occurrence's index would hide a - caller bug. + serialization), then finalized 0..n-1. Concurrent reorder() calls on one connection are serialized by a connection-scoped asyncio lock (shared across every ChildrenDao @@ -205,10 +212,10 @@ class ChildrenDao: transaction() rejects a second concurrent transaction with RuntimeError, so racing callers would crash instead of queueing. The lock makes the second call wait, then run - against the first call's final state. + against the first call's final state — which also keeps the + completeness check honest: the re-read of the table happens + inside the same lock and transaction as the writes. """ - if not ordered_ids: - return if len(set(ordered_ids)) != len(ordered_ids): raise ValueError( "reorder() received duplicate child ids; every id must " @@ -216,6 +223,29 @@ class ChildrenDao: ) async with _connection_lock(self._database): async with self._database.transaction(): + rows = await self._database.fetch_all( + f"SELECT id FROM children" + ) + existing = {row[0] for row in rows} + listed = set(ordered_ids) + missing = sorted(existing - listed) + unknown = sorted(listed - existing) + if missing or unknown: + details = [] + if missing: + details.append( + f"missing child ids {missing} (a partial " + "reorder would leave them with stale " + "positions)" + ) + if unknown: + details.append( + f"unknown child ids {unknown}" + ) + raise ValueError( + "reorder() must list every child exactly once; " + + "; ".join(details) + ) offset = len(ordered_ids) await self._database.execute_many( "UPDATE children SET sort_order = ? WHERE id = ?", diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index e3d28f6..cfc0799 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -12,6 +12,7 @@ from custom_components.nestquest.children import ( create_child, edit_child, list_children, + reorder_children, set_child_active, ) from custom_components.nestquest.dao_children import ChildRecord, ChildrenDao @@ -441,6 +442,114 @@ def test_set_child_active_missing_raises(tmp_path) -> None: _with_db(tmp_path, "set-active-missing.db")(_body) +# --------------------------------------------------------------------------- +# reorder_children: complete permutation or nothing changes +# --------------------------------------------------------------------------- + + +def test_reorder_children_persists_display_order(tmp_path) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + third = await create_child(database, "Cleo", sort_order=2) + await reorder_children(database, [third.id, first.id, second.id]) + # The active list returns children in the new sort order. + listed = await list_children(database, active_only=True) + assert [c.id for c in listed] == [third.id, first.id, second.id] + assert [c.sort_order for c in listed] == [0, 1, 2] + return listed + + _with_db(tmp_path, "reorder-round-trip.db")(_body) + + +def test_reorder_children_partial_list_rejected_order_unchanged( + tmp_path, +) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + third = await create_child(database, "Cleo", sort_order=2) + with pytest.raises(ValueError, match="missing child ids"): + await reorder_children(database, [third.id, first.id]) + listed = await list_children(database) + # Nothing changed: rejection happens before any write. + assert [c.id for c in listed] == [first.id, second.id, third.id] + assert [c.sort_order for c in listed] == [0, 1, 2] + return listed + + _with_db(tmp_path, "reorder-partial.db")(_body) + + +def test_reorder_children_unknown_id_rejected_order_unchanged( + tmp_path, +) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + with pytest.raises(ValueError, match="unknown child ids"): + await reorder_children(database, [second.id, first.id, 999]) + listed = await list_children(database) + assert [c.id for c in listed] == [first.id, second.id] + assert [c.sort_order for c in listed] == [0, 1] + return listed + + _with_db(tmp_path, "reorder-unknown.db")(_body) + + +def test_reorder_children_empty_list_rejected_when_children_exist( + tmp_path, +) -> None: + """An empty list is the maximally partial reorder: rejected while + any child exists (against an empty table it is a complete + permutation and stays a harmless no-op).""" + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + with pytest.raises(ValueError, match="missing child ids"): + await reorder_children(database, []) + assert [c.sort_order for c in await list_children(database)] == [0] + return None + + _with_db(tmp_path, "reorder-empty.db")(_body) + + +def test_reorder_children_duplicate_and_non_int_rejected(tmp_path) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + with pytest.raises(ValueError, match="duplicate child ids"): + await reorder_children(database, [first.id, first.id, second.id]) + with pytest.raises(ValueError, match="only integer child ids"): + await reorder_children(database, [first.id, second.id, True]) + with pytest.raises(ValueError, match="only integer child ids"): + await reorder_children(database, [first.id, second.id, "3"]) + listed = await list_children(database) + assert [c.sort_order for c in listed] == [0, 1] + return listed + + _with_db(tmp_path, "reorder-bad-inputs.db")(_body) + + +def test_reorder_children_inactive_children_included(tmp_path) -> None: + """Inactive children keep their display position: the permutation + covers the whole table, not just the active list.""" + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + await set_child_active(database, second.id, False) + # Both ids are required even though one is inactive. + await reorder_children(database, [second.id, first.id]) + listed = await list_children(database) + assert [c.id for c in listed] == [second.id, first.id] + assert [c.sort_order for c in listed] == [0, 1] + # The active list skips the inactive child but keeps position. + assert [c.id for c in await list_children(database, active_only=True)] == [ + first.id + ] + return listed + + _with_db(tmp_path, "reorder-inactive.db")(_body) + + @pytest.mark.parametrize("bad", ["1", "0", 1, 0, 1.0, None, "true"]) def test_set_child_active_rejects_non_bool(tmp_path, bad) -> None: """The DAO's int() would silently coerce "1"/0/1.0 into a state the diff --git a/tests/test_dao_children.py b/tests/test_dao_children.py index e3d78b0..893b855 100644 --- a/tests/test_dao_children.py +++ b/tests/test_dao_children.py @@ -227,24 +227,34 @@ def test_reorder_is_transactional_and_idempotent(tmp_path) -> None: _run(database.close()) -def test_reorder_empty_list_is_noop(tmp_path) -> None: +def test_reorder_empty_list_is_rejected(tmp_path) -> None: + """An empty list is the maximally partial reorder: rejected while + any child exists (Feature 03 reorder contract).""" + import pytest as pytest_module + database = _open_db(tmp_path / "reorder-empty.db") try: dao, _ = _daos(database) _make_child(dao, "Ada") - _run(dao.reorder([])) + with pytest_module.raises(ValueError, match="missing child ids"): + _run(dao.reorder([])) children = _run(dao.list_all()) assert children[0].sort_order == 0 finally: _run(database.close()) -def test_reorder_unknown_id_leaves_it_untouched(tmp_path) -> None: +def test_reorder_unknown_id_is_rejected(tmp_path) -> None: + """An unknown id is rejected before any write; the order is + untouched (Feature 03 reorder contract).""" + import pytest as pytest_module + database = _open_db(tmp_path / "reorder-unknown.db") try: dao, _ = _daos(database) a = _make_child(dao, "Ada") - _run(dao.reorder([a.id, 999])) + with pytest_module.raises(ValueError, match="unknown child ids"): + _run(dao.reorder([a.id, 999])) children = _run(dao.list_all()) assert children[0].sort_order == 0 finally: From 5fe6cb72abab9f29c3db4867667b9c409c20cb65 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:45:36 -0400 Subject: [PATCH 10/18] Review fix: strict non-bool int child_id validation on all three calls 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. --- custom_components/nestquest/children.py | 22 +++++++++++++----- tests/test_children_registry.py | 31 +++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 0633774..f4d8c3e 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -73,6 +73,18 @@ def _validate_sort_order(value: object) -> int: return value +def _validate_child_id(value: object) -> int: + """Reject non-int child ids (bools included) before any lookup. + + SQLite binds Python bools as integers, so ``True`` would silently + address child 1 and a float would round — malformed service input + must never mutate the wrong profile. + """ + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"child_id must be an integer, got {value!r}") + return value + + async def _warn_on_duplicate_name( database: NestQuestDatabase, display_name: str, @@ -166,6 +178,7 @@ async def edit_child( and its readback — the returned record is exactly the one this edit produced, not a later writer's. """ + _validate_child_id(child_id) async with _connection_lock(database): dao = ChildrenDao(database) existing = await dao.get(child_id) @@ -222,6 +235,7 @@ async def set_child_active( connection-scoped lock, so a concurrent opposite transition cannot make this call report the other caller's value. """ + _validate_child_id(child_id) async with _connection_lock(database): dao = ChildrenDao(database) if await dao.get(child_id) is None: @@ -246,17 +260,13 @@ async def reorder_children( existing child), an unknown id, or a duplicate is rejected with ValueError before any write, so the current order survives a rejected call. The DAO enforces this inside its own transaction - (check and write are atomic under the connection lock); this + (check and write are atomic under the connection lock); this wrapper additionally rejects non-integer ids up front, since ``bool`` and floats would silently compare against stored integer ids at the SQL layer. """ for child_id in ordered_ids: - if isinstance(child_id, bool) or not isinstance(child_id, int): - raise ValueError( - f"ordered_ids must contain only integer child ids, got " - f"{child_id!r}" - ) + _validate_child_id(child_id) await ChildrenDao(database).reorder(ordered_ids) diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index cfc0799..6b84a18 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -242,6 +242,33 @@ def test_edit_child_missing_raises(tmp_path) -> None: _with_db(tmp_path, "edit-missing.db")(_body) +@pytest.mark.parametrize("bad", [True, False, 1.0, "1", None]) +def test_edit_child_non_int_id_rejected(tmp_path, bad) -> None: + """SQLite binds bools as integers: True must not address child 1, + and floats must not round onto a real profile.""" + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="child_id must be an integer"): + await edit_child(database, bad, display_name="X") + # The refused edit left the profile untouched. + assert (await list_children(database))[0].display_name == "Ada" + return None + + _with_db(tmp_path, "edit-bad-id.db")(_body) + + +@pytest.mark.parametrize("bad", [True, False, 1.0, "1", None]) +def test_set_child_active_non_int_id_rejected(tmp_path, bad) -> None: + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="child_id must be an integer"): + await set_child_active(database, bad, False) + assert (await list_children(database))[0].is_active is True + return None + + _with_db(tmp_path, "set-active-bad-id.db")(_body) + + def test_edit_child_name_validation(tmp_path) -> None: async def _body(database): child = await create_child(database, "Ada") @@ -518,9 +545,9 @@ def test_reorder_children_duplicate_and_non_int_rejected(tmp_path) -> None: second = await create_child(database, "Bo", sort_order=1) with pytest.raises(ValueError, match="duplicate child ids"): await reorder_children(database, [first.id, first.id, second.id]) - with pytest.raises(ValueError, match="only integer child ids"): + with pytest.raises(ValueError, match="child_id must be an integer"): await reorder_children(database, [first.id, second.id, True]) - with pytest.raises(ValueError, match="only integer child ids"): + with pytest.raises(ValueError, match="child_id must be an integer"): await reorder_children(database, [first.id, second.id, "3"]) listed = await list_children(database) assert [c.sort_order for c in listed] == [0, 1] From 0d61ea2c8875c2cd99d5988782c1e04b07d475e7 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:03:53 -0400 Subject: [PATCH 11/18] Admin allowlist storage seeded with the setup user (Feature 03, task 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- custom_components/nestquest/__init__.py | 30 +- .../nestquest/admin_allowlist.py | 125 ++++++++ custom_components/nestquest/const.py | 6 + custom_components/nestquest/options_flow.py | 179 +++++++++-- custom_components/nestquest/strings.json | 9 +- .../nestquest/translations/en.json | 9 +- tests/conftest.py | 19 +- tests/test_admin_allowlist.py | 280 ++++++++++++++++++ tests/test_options_flow.py | 210 ++++++++++++- 9 files changed, 828 insertions(+), 39 deletions(-) create mode 100644 custom_components/nestquest/admin_allowlist.py create mode 100644 tests/test_admin_allowlist.py diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 65693e3..dbc26e9 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -13,10 +13,11 @@ from homeassistant.config_entries import ConfigEntry from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryNotReady -from .const import DOMAIN, LOGGER +from .const import CONF_ADMIN_USER_IDS, DOMAIN, LOGGER from .db import NestQuestDatabase from .migrations import apply_migrations from .store import async_get_db_path +from .admin_allowlist import seed_setup_admin _LOGGER = LOGGER @@ -245,6 +246,17 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: db = await _async_open_database( hass, await async_get_db_path(hass) ) + context = getattr(entry, "context", None) + context_user_id = ( + context.get("user_id") + if isinstance(context, dict) + else None + ) + await seed_setup_admin( + db, + stored_admin_ids=entry.data.get(CONF_ADMIN_USER_IDS), + context_user_id=context_user_id, + ) existing.database = db entry.runtime_data = existing return True @@ -257,6 +269,22 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: database = await _async_open_database(hass, await async_get_db_path(hass)) try: + # Seed the admin allowlist on first setup so the owner is never + # locked out: an empty database takes the config entry's stored + # admin copy (disaster recovery), else the HA user who + # completed the config flow. Non-empty allowlists are never + # touched (fail-closed narrowing is deliberate). + context = getattr(entry, "context", None) + context_user_id = ( + context.get("user_id") + if isinstance(context, dict) + else None + ) + await seed_setup_admin( + database, + stored_admin_ids=entry.data.get(CONF_ADMIN_USER_IDS), + context_user_id=context_user_id, + ) remove_update_listener = entry.add_update_listener(_async_update_listener) except BaseException: await database.close() diff --git a/custom_components/nestquest/admin_allowlist.py b/custom_components/nestquest/admin_allowlist.py new file mode 100644 index 0000000..6c4b3dc --- /dev/null +++ b/custom_components/nestquest/admin_allowlist.py @@ -0,0 +1,125 @@ +"""Business layer for the admin allowlist (Feature 03). + +Sits between the options flow / Feature 09 service gate and the typed +:class:`~.dao_children.AdminUsersDao`. The allowlist stores Home +Assistant user IDs — never usernames or display names (feature +guardrail) — and an empty allowlist must fail closed: every function +here refuses to write an empty allowlist, so the owner can never be +accidentally locked out of admin operations by a bad save. + +Shape policy: a user id is a non-empty trimmed string. Duplicate ids +in one write are rejected up front — they would silently map to one +row and hide a caller bug. Writes replace the allowlist atomically: +the diff (additions and removals) is computed and applied inside one +transaction under the connection-scoped lock shared with the DAOs, so +a concurrent reader never observes a half-applied sync. + +Seed policy (:func:`seed_setup_admin`): the allowlist is seeded only +when it is EMPTY, so re-running setup never resurrects a deliberately +narrowed list. The setup user (the HA user who completed the config +flow, via the entry context) is added automatically so the owner is +never locked out on first setup; a stored entry-data copy of the list +re-seeds a lost database. +""" +from __future__ import annotations + +import datetime +import logging + +from .dao_children import AdminUsersDao, _connection_lock +from .db import NestQuestDatabase + +LOGGER = logging.getLogger(__name__) + + +def _now_stamp() -> str: + """UTC second-precision stamp, matching the registry's policy.""" + return datetime.datetime.now(datetime.timezone.utc).isoformat( + timespec="seconds" + ) + + +def _validate_user_id(value: object) -> str: + """Return ``value`` trimmed; raise unless a non-empty string.""" + if not isinstance(value, str): + raise ValueError( + f"admin user id must be a string, got {value!r}" + ) + trimmed = value.strip() + if not trimmed: + raise ValueError("admin user id must not be empty") + return trimmed + + +async def list_admin_ids(database: NestQuestDatabase) -> list[str]: + """Return the allowlisted HA user IDs in storage order.""" + return [row.ha_user_id for row in await AdminUsersDao(database).list()] + + +async def set_admin_ids( + database: NestQuestDatabase, user_ids: list[str] +) -> None: + """Replace the allowlist with exactly ``user_ids`` (atomic sync). + + Refuses an empty list — an empty allowlist must fail closed, and + the only way to reach one through this API would be removing the + last admin, so that removal is refused here with a clear error. + Refuses non-string, empty, or duplicate ids up front. The diff + against the current allowlist (additions and removals) applies + inside one transaction under the connection lock. + """ + if not isinstance(user_ids, list): + raise ValueError("user_ids must be a list of HA user id strings") + normalized: list[str] = [] + for value in user_ids: + normalized.append(_validate_user_id(value)) + if len(set(normalized)) != len(normalized): + raise ValueError("user_ids must not contain duplicates") + if not normalized: + raise ValueError( + "refusing to empty the admin allowlist: at least one admin " + "must remain (an empty allowlist fails closed)" + ) + dao = AdminUsersDao(database) + async with _connection_lock(database): + async with database.transaction(): + current = {row.ha_user_id for row in await dao.list()} + target = set(normalized) + for user_id in sorted(current - target): + await dao.remove(user_id) + for user_id in normalized: + await dao.add(user_id, _now_stamp()) + + +async def seed_setup_admin( + database: NestQuestDatabase, + *, + stored_admin_ids: list[str] | None = None, + context_user_id: str | None = None, +) -> list[str]: + """Seed an empty allowlist so the owner is never locked out. + + Only acts when the allowlist is EMPTY. Preference order: the + stored entry-data copy of the list (disaster recovery for a lost + database), then the HA user who completed the config flow + (``context_user_id``). Returns the allowlist after seeding. + Non-empty allowlists are returned untouched — re-running setup + must never resurrect a deliberately narrowed list. + """ + dao = AdminUsersDao(database) + async with _connection_lock(database): + current = [row.ha_user_id for row in await dao.list()] + if current: + return current + candidates: list[str] = [] + if stored_admin_ids is not None: + if not isinstance(stored_admin_ids, list): + raise ValueError( + "stored_admin_ids must be a list of HA user id strings" + ) + candidates = [_validate_user_id(value) for value in stored_admin_ids] + elif context_user_id is not None: + candidates = [_validate_user_id(context_user_id)] + for user_id in candidates: + await dao.add(user_id, _now_stamp()) + return [row.ha_user_id for row in await dao.list()] diff --git a/custom_components/nestquest/const.py b/custom_components/nestquest/const.py index b0b4daa..8167e53 100644 --- a/custom_components/nestquest/const.py +++ b/custom_components/nestquest/const.py @@ -21,6 +21,12 @@ DEFAULT_DAY_ROLLOVER_TIME = "00:00" CONF_PANEL_IDLE_TIMEOUT = "panel_idle_timeout" DEFAULT_PANEL_IDLE_TIMEOUT = 300 +# Admin allowlist (Feature 03): HA user IDs permitted to call admin-only +# services. Stored as IDs, never usernames (feature guardrail). The +# database allowlist is authoritative; the config entry carries the same +# list as a disaster-recovery copy so a lost database can be re-seeded. +CONF_ADMIN_USER_IDS = "admin_user_ids" + # Quest windows (D-008): a definition may declare several day windows; # each produces its own instance per day and groups the panel's Quest # Log into three columns. The clock ranges are stored here so they stay diff --git a/custom_components/nestquest/options_flow.py b/custom_components/nestquest/options_flow.py index eeacafb..c106723 100644 --- a/custom_components/nestquest/options_flow.py +++ b/custom_components/nestquest/options_flow.py @@ -9,13 +9,16 @@ from homeassistant import config_entries from homeassistant.data_entry_flow import FlowResult from .const import ( + CONF_ADMIN_USER_IDS, CONF_DAY_ROLLOVER_TIME, CONF_HORIZON_DAYS, CONF_PANEL_IDLE_TIMEOUT, DEFAULT_DAY_ROLLOVER_TIME, DEFAULT_HORIZON_DAYS, DEFAULT_PANEL_IDLE_TIMEOUT, + DOMAIN, ) +from .admin_allowlist import list_admin_ids, set_admin_ids MIN_HORIZON_DAYS = 1 MIN_PANEL_IDLE_TIMEOUT = 30 @@ -28,6 +31,29 @@ def _parse_time(value: str) -> bool: return _TIME_PATTERN.fullmatch(value) is not None +def _multi_select(choices: dict[str, str]): + """A voluptuous validator for a multi-select of ``choices``. + + The mock-only test harness cannot import HA's config-validation + helpers (they pull in real HA), so this mirrors the shape of + ``homeassistant.helpers.config_validation.multi_select``: accepts a + list of keys from ``choices``, returns the list unchanged, rejects + anything else (None becomes the empty selection). + """ + + def _validate(value): + if value is None: + return [] + if not isinstance(value, list): + raise vol.Invalid("expected a list of selections") + for item in value: + if item not in choices: + raise vol.Invalid(f"{item!r} is not a valid choice") + return list(value) + + return _validate + + def _async_validate(user_input: dict[str, Any]) -> dict[str, str]: """Validate user input and return field-level errors instead of raising.""" errors: dict[str, str] = {} @@ -55,51 +81,154 @@ def _async_validate(user_input: dict[str, Any]) -> dict[str, str]: return errors -def _build_schema(current: dict[str, Any]) -> vol.Schema: - """Build the form schema, pre-filled with the current values.""" - return vol.Schema( - { - vol.Required( - CONF_HORIZON_DAYS, default=current[CONF_HORIZON_DAYS] - ): int, - vol.Required( - CONF_DAY_ROLLOVER_TIME, default=current[CONF_DAY_ROLLOVER_TIME] - ): str, - vol.Required( - CONF_PANEL_IDLE_TIMEOUT, default=current[CONF_PANEL_IDLE_TIMEOUT] - ): int, - } - ) +def _build_schema( + current: dict[str, Any], + *, + admin_choices: dict[str, str] | None = None, + admin_default: list[str] | None = None, +) -> vol.Schema: + """Build the form schema, pre-filled with the current values. + + The admin picker is a multi-select of EXISTING Home Assistant users + (never a free-text field — the allowlist stores user IDs); its + default is the current database allowlist, so an untouched submit + re-saves exactly who is already allowed. + """ + fields: dict[Any, Any] = { + vol.Required( + CONF_HORIZON_DAYS, default=current[CONF_HORIZON_DAYS] + ): int, + vol.Required( + CONF_DAY_ROLLOVER_TIME, default=current[CONF_DAY_ROLLOVER_TIME] + ): str, + vol.Required( + CONF_PANEL_IDLE_TIMEOUT, default=current[CONF_PANEL_IDLE_TIMEOUT] + ): int, + } + if admin_choices is not None: + fields[ + vol.Optional( + CONF_ADMIN_USER_IDS, default=list(admin_default or []) + ) + ] = _multi_select(admin_choices) + return vol.Schema(fields) class NestQuestOptionsFlow(config_entries.OptionsFlowWithConfigEntry): """Handle the options flow for NestQuest.""" + def _async_database(self): + """Return the runtime database for this entry, or None. + + Real HA only opens the options flow for an existing entry, so + the runtime record (and its database) exists; the None branch + covers direct construction in tests and degraded states. + """ + hass_data = getattr(self.hass, "data", None) + if not isinstance(hass_data, dict): + return None + record = hass_data.get(DOMAIN, {}).get(self.config_entry.entry_id) + database = getattr(record, "database", None) + return database if getattr(database, "connected", False) else None + + async def _async_admin_choices(self) -> dict[str, str]: + """Map HA user id -> display name for the picker. + + Real Home Assistant always provides ``hass.auth``; a missing + one (direct construction in tests, degraded harness) degrades + to an empty picker rather than crashing the form. + """ + auth = getattr(self.hass, "auth", None) + if auth is None: + return {} + users = await auth.async_get_users() + return { + user.id: user.name + for user in users + if getattr(user, "id", None) + } + + async def _async_current_admin_ids(self) -> list[str]: + """Return the current allowlist, or [] without a database.""" + database = self._async_database() + if database is None: + return [] + return await list_admin_ids(database) + async def async_step_init( self, user_input: dict[str, Any] | None = None ) -> FlowResult: """Manage the NestQuest options.""" + admin_choices = await self._async_admin_choices() + admin_default = await self._async_current_admin_ids() if user_input is None: return self.async_show_form( - step_id="init", data_schema=_build_schema(self._current_values()) + step_id="init", + data_schema=_build_schema( + self._current_values(), + admin_choices=admin_choices, + admin_default=admin_default, + ), ) errors = _async_validate(user_input) + # The admin ids are validated only when the key is present: + # schema-validated submits always carry it (vol.Optional's + # default fills from the current allowlist), while raw callers + # omitting the key keep the field untouched. + admin_ids_present = CONF_ADMIN_USER_IDS in user_input + admin_ids = user_input.get(CONF_ADMIN_USER_IDS) if errors: return self.async_show_form( step_id="init", - data_schema=_build_schema(self._current_values()), + data_schema=_build_schema( + self._current_values(), + admin_choices=admin_choices, + admin_default=admin_default, + ), + errors=errors, + ) + if admin_ids_present and ( + not isinstance(admin_ids, list) + or any(not isinstance(value, str) for value in admin_ids) + ): + errors[CONF_ADMIN_USER_IDS] = "invalid_admin" + elif admin_ids_present and not admin_ids: + # An empty allowlist fails closed: saving zero admins is + # refused, never silently applied. + errors[CONF_ADMIN_USER_IDS] = "no_admins" + elif admin_ids_present and any( + value not in admin_choices for value in admin_ids + ): + # The schema enforces membership for real HA submits; this + # backs the raw-input path up so an unknown id can never + # reach the allowlist. + errors[CONF_ADMIN_USER_IDS] = "invalid_admin" + if errors: + return self.async_show_form( + step_id="init", + data_schema=_build_schema( + self._current_values(), + admin_choices=admin_choices, + admin_default=admin_default, + ), errors=errors, ) - return self.async_create_entry( - title="NestQuest", - data={ - CONF_HORIZON_DAYS: user_input[CONF_HORIZON_DAYS], - CONF_DAY_ROLLOVER_TIME: user_input[CONF_DAY_ROLLOVER_TIME], - CONF_PANEL_IDLE_TIMEOUT: user_input[CONF_PANEL_IDLE_TIMEOUT], - }, - ) + # Apply the picker's selection to the allowlist when the + # runtime database is reachable; the selection is always + # stored on the entry as the disaster-recovery copy. + database = self._async_database() + if database is not None and admin_ids_present: + await set_admin_ids(database, admin_ids) + data = { + CONF_HORIZON_DAYS: user_input[CONF_HORIZON_DAYS], + CONF_DAY_ROLLOVER_TIME: user_input[CONF_DAY_ROLLOVER_TIME], + CONF_PANEL_IDLE_TIMEOUT: user_input[CONF_PANEL_IDLE_TIMEOUT], + } + if admin_ids_present: + data[CONF_ADMIN_USER_IDS] = admin_ids + return self.async_create_entry(title="NestQuest", data=data) def _current_values(self) -> dict[str, Any]: """Resolve current values from entry options, then entry data, then defaults.""" diff --git a/custom_components/nestquest/strings.json b/custom_components/nestquest/strings.json index 277492d..a37a39a 100644 --- a/custom_components/nestquest/strings.json +++ b/custom_components/nestquest/strings.json @@ -22,13 +22,16 @@ "data": { "horizon_days": "Generation horizon (days)", "day_rollover_time": "Day rollover time (HH:MM)", - "panel_idle_timeout": "Panel idle timeout (seconds)" + "panel_idle_timeout": "Panel idle timeout (seconds)", + "admin_user_ids": "Admin users (parents)" } } }, "errors": { "invalid": "Invalid value.", - "invalid_time": "Time must use 24-hour HH:MM format." + "invalid_time": "Time must use 24-hour HH:MM format.", + "no_admins": "Select at least one admin user; an empty allowlist would lock everyone out.", + "invalid_admin": "Invalid admin selection." } } -} \ No newline at end of file +} diff --git a/custom_components/nestquest/translations/en.json b/custom_components/nestquest/translations/en.json index 277492d..a37a39a 100644 --- a/custom_components/nestquest/translations/en.json +++ b/custom_components/nestquest/translations/en.json @@ -22,13 +22,16 @@ "data": { "horizon_days": "Generation horizon (days)", "day_rollover_time": "Day rollover time (HH:MM)", - "panel_idle_timeout": "Panel idle timeout (seconds)" + "panel_idle_timeout": "Panel idle timeout (seconds)", + "admin_user_ids": "Admin users (parents)" } } }, "errors": { "invalid": "Invalid value.", - "invalid_time": "Time must use 24-hour HH:MM format." + "invalid_time": "Time must use 24-hour HH:MM format.", + "no_admins": "Select at least one admin user; an empty allowlist would lock everyone out.", + "invalid_admin": "Invalid admin selection." } } -} \ No newline at end of file +} diff --git a/tests/conftest.py b/tests/conftest.py index da8c42a..2e67bfd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -218,12 +218,18 @@ def make_config_entry( entry_id: str = "test_entry", options: dict | None = None, data: dict | None = None, + context: dict | None = None, ): - """Create a stub config entry shaped like HA's ConfigEntry for tests.""" + """Create a stub config entry shaped like HA's ConfigEntry for tests. + + ``context`` mirrors HA's entry context (the flow that created the + entry carries the acting HA user under ``context["user_id"]``). + """ entry = SimpleNamespace( entry_id=entry_id, options=dict(options or {}), data=dict(data or {}), + context=dict(context or {}), ) entry.add_update_listener = None return entry @@ -302,8 +308,15 @@ async def hass(): def make_entry(): """Provide the config-entry factory as a fixture.""" - def _factory(entry_id: str = "test_entry", options: dict | None = None, data: dict | None = None): - return make_config_entry(entry_id=entry_id, options=options, data=data) + def _factory( + entry_id: str = "test_entry", + options: dict | None = None, + data: dict | None = None, + context: dict | None = None, + ): + return make_config_entry( + entry_id=entry_id, options=options, data=data, context=context + ) return _factory diff --git a/tests/test_admin_allowlist.py b/tests/test_admin_allowlist.py new file mode 100644 index 0000000..f1ddf81 --- /dev/null +++ b/tests/test_admin_allowlist.py @@ -0,0 +1,280 @@ +"""Tests for admin_allowlist.py: the Feature 03 allowlist business layer.""" +from __future__ import annotations + +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from custom_components.nestquest import async_setup_entry, async_unload_entry +from custom_components.nestquest.admin_allowlist import ( + list_admin_ids, + seed_setup_admin, + set_admin_ids, +) +from custom_components.nestquest.dao_children import AdminUsersDao +from custom_components.nestquest.db import NestQuestDatabase +from custom_components.nestquest.migrations import apply_migrations + + +def _run(coro): + return asyncio.new_event_loop().run_until_complete(coro) + + +def _make_hass_mock(): + hass = MagicMock() + hass.async_add_executor_job = AsyncMock(side_effect=(lambda fn, *a: fn(*a))) + return hass + + +def _with_db(tmp_path, name): + def _run_test(body): + async def _main(): + database = NestQuestDatabase(_make_hass_mock()) + await database.open(tmp_path / name) + try: + await apply_migrations(database) + return await body(database) + finally: + await database.close() + + return _run(_main()) + + return _run_test + + +# --------------------------------------------------------------------------- +# set_admin_ids: read/write with the fail-closed guarantee +# --------------------------------------------------------------------------- + + +def test_set_admin_ids_round_trip(tmp_path) -> None: + async def _body(database): + await set_admin_ids(database, ["user-owner", "user-parent"]) + # Membership is the contract; AdminUsersDao.list orders by id. + assert sorted(await list_admin_ids(database)) == [ + "user-owner", + "user-parent", + ] + # Replace: the removed id is gone, the new one present. + await set_admin_ids(database, ["user-parent", "user-grandma"]) + assert sorted(await list_admin_ids(database)) == [ + "user-grandma", + "user-parent", + ] + return await list_admin_ids(database) + + _with_db(tmp_path, "set-round-trip.db")(_body) + + +def test_set_admin_ids_rejects_empty_allowlist(tmp_path) -> None: + """Removing the last admin (or saving an empty selection) is + refused with a clear error: an empty allowlist fails closed, so + the owner must never be able to reach one through this API.""" + async def _body(database): + await set_admin_ids(database, ["user-owner"]) + with pytest.raises(ValueError, match="at least one admin"): + await set_admin_ids(database, []) + # The refused write left the allowlist intact. + assert await list_admin_ids(database) == ["user-owner"] + return None + + _with_db(tmp_path, "set-empty-refused.db")(_body) + + +@pytest.mark.parametrize( + "bad", [None, "notalist", [42], [None], [""], [" "], 5] +) +def test_set_admin_ids_rejects_malformed_input(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError): + await set_admin_ids(database, bad) + assert await list_admin_ids(database) == [] + return None + + _with_db(tmp_path, "set-malformed.db")(_body) + + +def test_set_admin_ids_rejects_duplicates(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="duplicates"): + await set_admin_ids(database, ["user-a", "user-a"]) + assert await list_admin_ids(database) == [] + return None + + _with_db(tmp_path, "set-duplicates.db")(_body) + + +def test_set_admin_ids_trims_and_replaces_atomically(tmp_path) -> None: + async def _body(database): + await set_admin_ids(database, [" user-a "]) + assert await list_admin_ids(database) == ["user-a"] + # Sync that both adds and removes in one write. + await set_admin_ids(database, ["user-b", "user-c"]) + assert await list_admin_ids(database) == ["user-b", "user-c"] + return None + + _with_db(tmp_path, "set-trim.db")(_body) + + +# --------------------------------------------------------------------------- +# seed_setup_admin: the owner is never locked out on first setup +# --------------------------------------------------------------------------- + + +def test_seed_adds_context_user_on_first_setup(tmp_path) -> None: + async def _body(database): + seeded = await seed_setup_admin( + database, stored_admin_ids=None, context_user_id="owner-1" + ) + assert seeded == ["owner-1"] + assert await list_admin_ids(database) == ["owner-1"] + return seeded + + _with_db(tmp_path, "seed-context.db")(_body) + + +def test_seed_prefers_stored_entry_ids_over_context(tmp_path) -> None: + async def _body(database): + seeded = await seed_setup_admin( + database, + stored_admin_ids=["user-a", "user-b"], + context_user_id="owner-1", + ) + assert seeded == ["user-a", "user-b"] + return seeded + + _with_db(tmp_path, "seed-stored.db")(_body) + + +def test_seed_leaves_nonempty_allowlist_untouched(tmp_path) -> None: + """Re-running setup must never resurrect a deliberately narrowed + list: only an EMPTY allowlist is seeded.""" + async def _body(database): + await set_admin_ids(database, ["user-kept"]) + seeded = await seed_setup_admin( + database, + stored_admin_ids=["user-a", "user-b"], + context_user_id="owner-1", + ) + assert seeded == ["user-kept"] + assert await list_admin_ids(database) == ["user-kept"] + return seeded + + _with_db(tmp_path, "seed-narrowed.db")(_body) + + +def test_seed_with_nothing_available_leaves_empty(tmp_path) -> None: + async def _body(database): + seeded = await seed_setup_admin( + database, stored_admin_ids=None, context_user_id=None + ) + assert seeded == [] + return seeded + + _with_db(tmp_path, "seed-nothing.db")(_body) + + +def test_seed_rejects_malformed_candidates(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError): + await seed_setup_admin( + database, stored_admin_ids=[42], context_user_id=None + ) + with pytest.raises(ValueError): + await seed_setup_admin( + database, stored_admin_ids=None, context_user_id=" " + ) + assert await list_admin_ids(database) == [] + return None + + _with_db(tmp_path, "seed-malformed.db")(_body) + + +# --------------------------------------------------------------------------- +# Setup wiring: async_setup_entry seeds automatically +# --------------------------------------------------------------------------- + + +async def _setup_entry(hass, entry, registry): + from tests.test_lifecycle import _wire + + entry = _wire(entry, registry) + await async_setup_entry(hass, entry) + return entry + + +async def test_setup_seeds_allowlist_from_entry_context( + hass, make_entry +) -> None: + entry = await _setup_entry( + hass, + make_entry(context={"user_id": "ha-owner"}), + hass.registry, + ) + database = entry.runtime_data.database + try: + assert await list_admin_ids(database) == ["ha-owner"] + finally: + await database.close() + assert await async_unload_entry(hass, entry) is True + + +async def test_setup_seeds_allowlist_from_stored_entry_data( + hass, make_entry +) -> None: + """A lost database re-seeded from the entry's stored admin copy.""" + from custom_components.nestquest.const import CONF_ADMIN_USER_IDS + + entry = await _setup_entry( + hass, + make_entry(data={CONF_ADMIN_USER_IDS: ["user-a", "user-b"]}), + hass.registry, + ) + database = entry.runtime_data.database + try: + assert await list_admin_ids(database) == ["user-a", "user-b"] + finally: + await database.close() + assert await async_unload_entry(hass, entry) is True + + +async def test_setup_without_context_leaves_allowlist_empty( + hass, make_entry +) -> None: + """Fail closed: no context user and no stored copy means an empty + allowlist — nobody is admin until the owner configures it.""" + entry = await _setup_entry(hass, make_entry(), hass.registry) + database = entry.runtime_data.database + try: + assert await list_admin_ids(database) == [] + finally: + await database.close() + assert await async_unload_entry(hass, entry) is True + + +async def test_setup_does_not_reseed_narrowed_allowlist( + hass, make_entry +) -> None: + from custom_components.nestquest import async_unload_entry + from custom_components.nestquest.const import CONF_ADMIN_USER_IDS + + entry = await _setup_entry( + hass, make_entry(context={"user_id": "ha-owner"}), hass.registry + ) + database = entry.runtime_data.database + await set_admin_ids(database, ["user-parent"]) + await database.close() + assert await async_unload_entry(hass, entry) is True + + # Reload with the SAME context: the narrowed list survives. + entry_reloaded = await _setup_entry( + hass, make_entry(context={"user_id": "ha-owner"}), hass.registry + ) + database_b = entry_reloaded.runtime_data.database + try: + assert await list_admin_ids(database_b) == ["user-parent"] + finally: + await database.close() + assert await async_unload_entry(hass, entry_reloaded) is True diff --git a/tests/test_options_flow.py b/tests/test_options_flow.py index 9b8dad8..b93b4f2 100644 --- a/tests/test_options_flow.py +++ b/tests/test_options_flow.py @@ -14,6 +14,7 @@ from conftest import make_config_entry, make_hass, options_flow_base as _options from custom_components.nestquest import config_flow from custom_components.nestquest import options_flow from custom_components.nestquest.const import ( + CONF_ADMIN_USER_IDS, CONF_DAY_ROLLOVER_TIME, CONF_HORIZON_DAYS, CONF_PANEL_IDLE_TIMEOUT, @@ -46,13 +47,48 @@ def _make_entry(entry_id: str = "test_entry", options: dict | None = None, data: return make_config_entry(entry_id=entry_id, options=options, data=data) -def _make_flow(entry) -> options_flow.NestQuestOptionsFlow: - """Create an options flow instance bound to a stubbed config entry.""" +def _make_flow(entry, *, users=None, database=None) -> options_flow.NestQuestOptionsFlow: + """Create an options flow instance bound to a stubbed config entry. + + ``users`` populates a stubbed hass.auth for the admin picker; + ``database`` wires the runtime record so the flow can read and + write the allowlist like real HA's options flow does. + """ + from unittest.mock import AsyncMock + flow = options_flow.NestQuestOptionsFlow(config_entry=entry) flow.hass = SimpleNamespace() + flow.hass.auth = SimpleNamespace( + async_get_users=AsyncMock(return_value=users or []) + ) + if database is not None: + from custom_components.nestquest.const import DOMAIN + + flow.hass.data = { + DOMAIN: {entry.entry_id: SimpleNamespace(database=database)} + } return flow +def _user(user_id: str, name: str): + return SimpleNamespace(id=user_id, name=name) + + +def _open_allowlist_db(tmp_path, name): + """A real migrated temp database for the flow to read and write.""" + from unittest.mock import AsyncMock, MagicMock + + from custom_components.nestquest.db import NestQuestDatabase + from custom_components.nestquest.migrations import apply_migrations + + hass = MagicMock() + hass.async_add_executor_job = AsyncMock(side_effect=(lambda fn, *a: fn(*a))) + database = NestQuestDatabase(hass) + _run(database.open(tmp_path / name)) + _run(apply_migrations(database)) + return database + + def _schema_values(schema) -> dict: """Extract the per-field default values from a real voluptuous schema.""" return { @@ -161,7 +197,12 @@ def test_options_flow_falls_back_to_entry_data() -> None: def test_options_flow_schema_applies_stored_defaults() -> None: - """A real voluptuous validate({}) fills every field from the stored defaults.""" + """A real voluptuous validate({}) fills every field from the stored defaults. + + Without a runtime database (and no auth in the bare harness), the + admin picker's default is the empty selection and its choice set is + empty — the three legacy settings are unaffected. + """ entry = _make_entry( options={ CONF_HORIZON_DAYS: 21, @@ -174,6 +215,7 @@ def test_options_flow_schema_applies_stored_defaults() -> None: CONF_HORIZON_DAYS: 21, CONF_DAY_ROLLOVER_TIME: "05:45", CONF_PANEL_IDLE_TIMEOUT: 120, + CONF_ADMIN_USER_IDS: [], } @@ -373,4 +415,164 @@ def test_options_flow_no_hardcoded_domain_or_db_filename() -> None: ) assert node.value != "nestquest.db", ( f"Hard-coded database filename in {py_file.name}:{node.lineno}" - ) \ No newline at end of file + ) + +# --------------------------------------------------------------------------- +# Admin allowlist picker (Feature 03) +# --------------------------------------------------------------------------- + + +def test_options_flow_picker_lists_existing_users(tmp_path) -> None: + """The picker offers EXISTING HA users (id -> name), never free text.""" + import voluptuous as vol + + database = _open_allowlist_db(tmp_path, "picker-users.db") + try: + from custom_components.nestquest.admin_allowlist import set_admin_ids + + _run(set_admin_ids(database, ["u1"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua"), _user("u2", "Sam")], + database=database, + ) + result = _run(flow.async_step_init(None)) + schema = result["data_schema"] + marker = next( + m for m in schema.schema + if getattr(m, "schema", None) == CONF_ADMIN_USER_IDS + ) + # Default is the CURRENT database allowlist. + values = _schema_values(schema) + assert values[CONF_ADMIN_USER_IDS] == ["u1"] + # Both users are selectable; an unknown id is rejected by the + # schema itself (this is what real HA submits go through). + validated = schema( + { + CONF_HORIZON_DAYS: 7, + CONF_DAY_ROLLOVER_TIME: "03:30", + CONF_PANEL_IDLE_TIMEOUT: 60, + CONF_ADMIN_USER_IDS: ["u1", "u2"], + } + ) + assert validated[CONF_ADMIN_USER_IDS] == ["u1", "u2"] + with pytest.raises(vol.Invalid): + schema( + { + CONF_HORIZON_DAYS: 7, + CONF_DAY_ROLLOVER_TIME: "03:30", + CONF_PANEL_IDLE_TIMEOUT: 60, + CONF_ADMIN_USER_IDS: ["impostor"], + } + ) + finally: + _run(database.close()) + + +def test_options_flow_submit_applies_selection_to_database(tmp_path) -> None: + database = _open_allowlist_db(tmp_path, "picker-apply.db") + try: + from custom_components.nestquest.admin_allowlist import ( + list_admin_ids, + set_admin_ids, + ) + + _run(set_admin_ids(database, ["u1"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua"), _user("u2", "Sam")], + database=database, + ) + result = _run( + flow.async_step_init({**VALID_INPUT, CONF_ADMIN_USER_IDS: ["u2"]}) + ) + assert result["type"] == "create_entry" + assert result["data"][CONF_ADMIN_USER_IDS] == ["u2"] + # The allowlist (and the entry's recovery copy) carry the pick. + assert await_gather(list_admin_ids(database)) == ["u2"] + finally: + _run(database.close()) + + +def await_gather(coro): + return _run(coro) + + +def test_options_flow_empty_selection_refused(tmp_path) -> None: + """Saving zero admins is refused: an empty allowlist fails closed.""" + database = _open_allowlist_db(tmp_path, "picker-empty.db") + try: + from custom_components.nestquest.admin_allowlist import ( + list_admin_ids, + set_admin_ids, + ) + + _run(set_admin_ids(database, ["u1"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua")], + database=database, + ) + result = _run( + flow.async_step_init({**VALID_INPUT, CONF_ADMIN_USER_IDS: []}) + ) + assert result["type"] == "form" + assert result["errors"] == {CONF_ADMIN_USER_IDS: "no_admins"} + assert await_gather(list_admin_ids(database)) == ["u1"] + finally: + _run(database.close()) + + +def test_options_flow_unknown_selection_refused(tmp_path) -> None: + database = _open_allowlist_db(tmp_path, "picker-unknown.db") + try: + from custom_components.nestquest.admin_allowlist import ( + list_admin_ids, + set_admin_ids, + ) + + _run(set_admin_ids(database, ["u1"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua")], + database=database, + ) + result = _run( + flow.async_step_init( + {**VALID_INPUT, CONF_ADMIN_USER_IDS: ["impostor"]} + ) + ) + assert result["type"] == "form" + assert result["errors"] == {CONF_ADMIN_USER_IDS: "invalid_admin"} + assert await_gather(list_admin_ids(database)) == ["u1"] + finally: + _run(database.close()) + + +def test_options_flow_submit_without_admin_key_keeps_allowlist(tmp_path) -> None: + """Raw callers omitting the key (pre-picker payloads) leave the + allowlist untouched and produce the legacy three-key entry.""" + database = _open_allowlist_db(tmp_path, "picker-legacy.db") + try: + from custom_components.nestquest.admin_allowlist import ( + list_admin_ids, + set_admin_ids, + ) + + _run(set_admin_ids(database, ["u1"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua")], + database=database, + ) + result = _run(flow.async_step_init(dict(VALID_INPUT))) + assert result["type"] == "create_entry" + assert CONF_ADMIN_USER_IDS not in result["data"] + assert await_gather(list_admin_ids(database)) == ["u1"] + finally: + _run(database.close()) From d90c17046009233542c61a1edc23f7b9efd77910 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:13:32 -0400 Subject: [PATCH 12/18] Review fixes: persisted flow-user admin seed and real cv.multi_select MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- custom_components/nestquest/__init__.py | 25 ++++++++++++------ custom_components/nestquest/config_flow.py | 21 +++++++++++++--- custom_components/nestquest/options_flow.py | 28 +++------------------ tests/conftest.py | 28 +++++++++++++++++++++ tests/test_admin_allowlist.py | 24 ++++++++++++++++++ tests/test_config_flow.py | 23 ++++++++++++++++- 6 files changed, 114 insertions(+), 35 deletions(-) diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index dbc26e9..688ae8a 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -242,10 +242,16 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: # record without a live connection (a previous setup's # close-on-failure, or a hand-replaced file) must get the # missing/valid/corrupt classification too, never a raw - # sqlite3 error and never skipped migrations. + # sqlite3 error and never skipped migrations — and the + # same allowlist seeding, so a reopened fresh file is + # never ownerless. db = await _async_open_database( hass, await async_get_db_path(hass) ) + stored_admin_ids = ( + entry.options.get(CONF_ADMIN_USER_IDS) + or entry.data.get(CONF_ADMIN_USER_IDS) + ) context = getattr(entry, "context", None) context_user_id = ( context.get("user_id") @@ -254,7 +260,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: ) await seed_setup_admin( db, - stored_admin_ids=entry.data.get(CONF_ADMIN_USER_IDS), + stored_admin_ids=stored_admin_ids, context_user_id=context_user_id, ) existing.database = db @@ -270,10 +276,15 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: database = await _async_open_database(hass, await async_get_db_path(hass)) try: # Seed the admin allowlist on first setup so the owner is never - # locked out: an empty database takes the config entry's stored - # admin copy (disaster recovery), else the HA user who - # completed the config flow. Non-empty allowlists are never - # touched (fail-closed narrowing is deliberate). + # locked out: an empty database takes the persisted admin copy + # — the options flow's saved list first (it is the current + # one), else the config flow's user from entry data — and only + # then the HA user still present on the entry context. No-op + # on a non-empty list (fail-closed narrowing is deliberate). + stored_admin_ids = ( + entry.options.get(CONF_ADMIN_USER_IDS) + or entry.data.get(CONF_ADMIN_USER_IDS) + ) context = getattr(entry, "context", None) context_user_id = ( context.get("user_id") @@ -282,7 +293,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: ) await seed_setup_admin( database, - stored_admin_ids=entry.data.get(CONF_ADMIN_USER_IDS), + stored_admin_ids=stored_admin_ids, context_user_id=context_user_id, ) remove_update_listener = entry.add_update_listener(_async_update_listener) diff --git a/custom_components/nestquest/config_flow.py b/custom_components/nestquest/config_flow.py index a8ccbc2..803b2f1 100644 --- a/custom_components/nestquest/config_flow.py +++ b/custom_components/nestquest/config_flow.py @@ -7,7 +7,7 @@ from homeassistant import config_entries from homeassistant.core import callback from homeassistant.data_entry_flow import FlowResult -from .const import DOMAIN +from .const import CONF_ADMIN_USER_IDS, DOMAIN class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): @@ -28,7 +28,14 @@ class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): async def async_step_user( self, user_input: dict[str, Any] | None = None ) -> FlowResult: - """Handle the user step of the config flow.""" + """Handle the user step of the config flow. + + The HA user completing the flow is persisted into the entry + data as the initial admin allowlist copy: real HA does not + guarantee the flow context survives to setup, and entry data is + the persisted record first setup seeds from — the owner is + never locked out. + """ await self.async_set_unique_id(DOMAIN, raise_on_progress=False) if self._async_in_progress() or self._async_current_entries(): return self.async_abort(reason="single_instance_allowed") @@ -36,4 +43,12 @@ class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): if user_input is None: return self.async_show_form(step_id="user") - return self.async_create_entry(title="NestQuest", data=user_input) \ No newline at end of file + data = dict(user_input) + flow_user_id = ( + self.context.get("user_id") + if isinstance(self.context, dict) + else None + ) + if flow_user_id: + data[CONF_ADMIN_USER_IDS] = [flow_user_id] + return self.async_create_entry(title="NestQuest", data=data) \ No newline at end of file diff --git a/custom_components/nestquest/options_flow.py b/custom_components/nestquest/options_flow.py index c106723..2efe70a 100644 --- a/custom_components/nestquest/options_flow.py +++ b/custom_components/nestquest/options_flow.py @@ -7,6 +7,7 @@ from typing import Any import voluptuous as vol from homeassistant import config_entries from homeassistant.data_entry_flow import FlowResult +from homeassistant.helpers import config_validation as cv from .const import ( CONF_ADMIN_USER_IDS, @@ -31,29 +32,6 @@ def _parse_time(value: str) -> bool: return _TIME_PATTERN.fullmatch(value) is not None -def _multi_select(choices: dict[str, str]): - """A voluptuous validator for a multi-select of ``choices``. - - The mock-only test harness cannot import HA's config-validation - helpers (they pull in real HA), so this mirrors the shape of - ``homeassistant.helpers.config_validation.multi_select``: accepts a - list of keys from ``choices``, returns the list unchanged, rejects - anything else (None becomes the empty selection). - """ - - def _validate(value): - if value is None: - return [] - if not isinstance(value, list): - raise vol.Invalid("expected a list of selections") - for item in value: - if item not in choices: - raise vol.Invalid(f"{item!r} is not a valid choice") - return list(value) - - return _validate - - def _async_validate(user_input: dict[str, Any]) -> dict[str, str]: """Validate user input and return field-level errors instead of raising.""" errors: dict[str, str] = {} @@ -106,11 +84,13 @@ def _build_schema( ): int, } if admin_choices is not None: + # HA's supported multi-select validator: the frontend can + # serialize the field as a real picker of the given choices. fields[ vol.Optional( CONF_ADMIN_USER_IDS, default=list(admin_default or []) ) - ] = _multi_select(admin_choices) + ] = cv.multi_select(admin_choices) return vol.Schema(fields) diff --git a/tests/conftest.py b/tests/conftest.py index 2e67bfd..dd0fc25 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -42,6 +42,8 @@ _HA_MODULES = ( "homeassistant.config_entries", "homeassistant.data_entry_flow", "homeassistant.exceptions", + "homeassistant.helpers", + "homeassistant.helpers.config_validation", ) def _find_spec(name: str): @@ -79,6 +81,8 @@ for _parent, _child in ( ("homeassistant", "data_entry_flow"), ("homeassistant", "core"), ("homeassistant", "exceptions"), + ("homeassistant", "helpers"), + ("homeassistant.helpers", "config_validation"), ): setattr(sys.modules[_parent], _child, sys.modules[f"{_parent}.{_child}"]) @@ -181,6 +185,30 @@ class ConfigEntryNotReady(Exception): _exceptions_mock.ConfigEntryNotReady = ConfigEntryNotReady +import voluptuous as vol + + +def _multi_select(choices: dict): + """Mirror homeassistant.helpers.config_validation.multi_select. + + The options flow's admin picker uses HA's supported multi-select + validator (schema-serializable by HA's frontend); the mock provides + the same contract: a list whose members are keys of ``choices``. + """ + + def _validate(selected): + if not isinstance(selected, list): + raise vol.Invalid("Not a list") + for value in selected: + if value not in choices: + raise vol.Invalid(f"{value} is not a valid option") + return selected + + return _validate + + +_ha_mock("homeassistant.helpers.config_validation").multi_select = _multi_select + # --------------------------------------------------------------------------- # Shared helpers for the behavior tests. diff --git a/tests/test_admin_allowlist.py b/tests/test_admin_allowlist.py index f1ddf81..4ddbf28 100644 --- a/tests/test_admin_allowlist.py +++ b/tests/test_admin_allowlist.py @@ -278,3 +278,27 @@ async def test_setup_does_not_reseed_narrowed_allowlist( finally: await database.close() assert await async_unload_entry(hass, entry_reloaded) is True + + +async def test_setup_seed_prefers_options_over_entry_data( + hass, make_entry +) -> None: + """Seed precedence on a fresh database: the options flow's saved + list (the current one) beats the config flow's original copy.""" + from custom_components.nestquest.const import CONF_ADMIN_USER_IDS + + entry = await _setup_entry( + hass, + make_entry( + options={CONF_ADMIN_USER_IDS: ["user-current"]}, + data={CONF_ADMIN_USER_IDS: ["ha-owner"]}, + context={"user_id": "ha-owner"}, + ), + hass.registry, + ) + database = entry.runtime_data.database + try: + assert await list_admin_ids(database) == ["user-current"] + finally: + await database.close() + assert await async_unload_entry(hass, entry) is True diff --git a/tests/test_config_flow.py b/tests/test_config_flow.py index b744bf4..246e59b 100644 --- a/tests/test_config_flow.py +++ b/tests/test_config_flow.py @@ -154,4 +154,25 @@ def test_no_hardcoded_domain_or_db_filename_in_new_modules() -> None: ) assert node.value != "nestquest.db", ( f"Found hard-coded database filename in {py_file.name}:{node.lineno}" - ) \ No newline at end of file + ) + +async def test_flow_persists_flow_user_as_initial_admin(hass, make_flow) -> None: + """The HA user completing the flow lands in entry data as the + initial admin copy: entry.context is not persisted by real HA, so + the durable record must carry the owner.""" + from custom_components.nestquest.const import CONF_ADMIN_USER_IDS + + flow = make_flow(hass) + flow.context = {"user_id": "ha-owner"} + result = await flow.async_step_user({}) + assert result["type"] == "create_entry" + assert result["data"] == {CONF_ADMIN_USER_IDS: ["ha-owner"]} + + +async def test_flow_without_context_user_keeps_data_untouched( + hass, make_flow +) -> None: + flow = make_flow(hass) + result = await flow.async_step_user({"horizon_days": 7}) + assert result["type"] == "create_entry" + assert result["data"] == {"horizon_days": 7} From f0fa39660c7a337d8d775871abe4cd0d66e98b2c Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:16:10 -0400 Subject: [PATCH 13/18] Review fix: duplicate admin selections surface as a field error 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. --- custom_components/nestquest/options_flow.py | 5 ++++ tests/test_options_flow.py | 29 +++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/custom_components/nestquest/options_flow.py b/custom_components/nestquest/options_flow.py index 2efe70a..9ab8214 100644 --- a/custom_components/nestquest/options_flow.py +++ b/custom_components/nestquest/options_flow.py @@ -184,6 +184,11 @@ class NestQuestOptionsFlow(config_entries.OptionsFlowWithConfigEntry): # backs the raw-input path up so an unknown id can never # reach the allowlist. errors[CONF_ADMIN_USER_IDS] = "invalid_admin" + elif admin_ids_present and len(set(admin_ids)) != len(admin_ids): + # set_admin_ids refuses duplicates with ValueError; a flow + # submission must surface that as a field error instead of + # an unhandled crash. + errors[CONF_ADMIN_USER_IDS] = "invalid_admin" if errors: return self.async_show_form( step_id="init", diff --git a/tests/test_options_flow.py b/tests/test_options_flow.py index b93b4f2..0830b5c 100644 --- a/tests/test_options_flow.py +++ b/tests/test_options_flow.py @@ -576,3 +576,32 @@ def test_options_flow_submit_without_admin_key_keeps_allowlist(tmp_path) -> None assert await_gather(list_admin_ids(database)) == ["u1"] finally: _run(database.close()) + + +def test_options_flow_duplicate_selection_refused(tmp_path) -> None: + """Duplicate ids surface as a field error, not an unhandled + ValueError from the business layer.""" + database = _open_allowlist_db(tmp_path, "picker-dupe.db") + try: + from custom_components.nestquest.admin_allowlist import ( + list_admin_ids, + set_admin_ids, + ) + + _run(set_admin_ids(database, ["u1"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua"), _user("u2", "Sam")], + database=database, + ) + result = _run( + flow.async_step_init( + {**VALID_INPUT, CONF_ADMIN_USER_IDS: ["u2", "u2"]} + ) + ) + assert result["type"] == "form" + assert result["errors"] == {CONF_ADMIN_USER_IDS: "invalid_admin"} + assert await_gather(list_admin_ids(database)) == ["u1"] + finally: + _run(database.close()) From 068ace5693af7a689368718d9ccb967ba64be277 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:22:39 -0400 Subject: [PATCH 14/18] Review fix: owner-account fallback seed for first setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4 (Codex gpt-5.6-terra, HIGH): relying on the flow context alone can leave first setup ownerless. seed_setup_admin gains an owner_ids tier: when no persisted copy and no context user exist, every HA owner account is seeded (owners can already do everything in HA — no privilege escalation). __init__ resolves owners via hass.auth.async_get_users on both setup paths; conftest's hass gains the auth surface. Tests: owner fallback seeds only owners, context user still wins over owners, owner_ids argument validation. Suite: 910 passed. --- custom_components/nestquest/__init__.py | 25 +++++++ .../nestquest/admin_allowlist.py | 21 ++++-- tests/conftest.py | 6 +- tests/test_admin_allowlist.py | 70 +++++++++++++++++++ 4 files changed, 116 insertions(+), 6 deletions(-) diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 688ae8a..d69e2d5 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -226,6 +226,28 @@ class NestQuestRuntimeData: remove_update_listener: Callable[[], Any] +async def _async_owner_user_ids(hass: HomeAssistant) -> list[str]: + """Return every Home Assistant owner-account user id. + + The last-resort admin seed: when no persisted allowlist copy and no + flow-context user survive to first setup, the HA owner accounts are + seeded so the household is never left with zero admins (an empty + allowlist fails closed, which would lock the household out of its + own integration). Owner accounts can already do everything in HA, + so granting NestQuest admin is not a privilege escalation. + """ + auth = getattr(hass, "auth", None) + if auth is None: + return [] + users = await auth.async_get_users() + owners = [ + user.id + for user in users + if getattr(user, "is_owner", False) and getattr(user, "id", None) + ] + return owners + + async def async_setup(hass: HomeAssistant, config: dict[str, Any]) -> bool: """Set up the NestQuest integration. YAML configuration is not used, returns True.""" return True @@ -262,6 +284,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: db, stored_admin_ids=stored_admin_ids, context_user_id=context_user_id, + owner_ids=await _async_owner_user_ids(hass), ) existing.database = db entry.runtime_data = existing @@ -291,10 +314,12 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: if isinstance(context, dict) else None ) + owner_ids = await _async_owner_user_ids(hass) await seed_setup_admin( database, stored_admin_ids=stored_admin_ids, context_user_id=context_user_id, + owner_ids=owner_ids, ) remove_update_listener = entry.add_update_listener(_async_update_listener) except BaseException: diff --git a/custom_components/nestquest/admin_allowlist.py b/custom_components/nestquest/admin_allowlist.py index 6c4b3dc..a672d2e 100644 --- a/custom_components/nestquest/admin_allowlist.py +++ b/custom_components/nestquest/admin_allowlist.py @@ -96,15 +96,20 @@ async def seed_setup_admin( *, stored_admin_ids: list[str] | None = None, context_user_id: str | None = None, + owner_ids: list[str] | None = None, ) -> list[str]: """Seed an empty allowlist so the owner is never locked out. Only acts when the allowlist is EMPTY. Preference order: the - stored entry-data copy of the list (disaster recovery for a lost - database), then the HA user who completed the config flow - (``context_user_id``). Returns the allowlist after seeding. - Non-empty allowlists are returned untouched — re-running setup - must never resurrect a deliberately narrowed list. + stored persisted copy of the list (the options flow's current + selection, then the config flow's original — disaster recovery for + a lost database), then the HA user who completed the config flow + (``context_user_id``), then — when no caller identity survived at + all — every Home Assistant OWNER account (``owner_ids``), so a + first setup whose flow context lost the user id still seeds a + working admin. Returns the allowlist after seeding. Non-empty + allowlists are returned untouched — re-running setup must never + resurrect a deliberately narrowed list. """ dao = AdminUsersDao(database) async with _connection_lock(database): @@ -120,6 +125,12 @@ async def seed_setup_admin( candidates = [_validate_user_id(value) for value in stored_admin_ids] elif context_user_id is not None: candidates = [_validate_user_id(context_user_id)] + elif owner_ids: + if not isinstance(owner_ids, list): + raise ValueError( + "owner_ids must be a list of HA user id strings" + ) + candidates = [_validate_user_id(value) for value in owner_ids] for user_id in candidates: await dao.add(user_id, _now_stamp()) return [row.ha_user_id for row in await dao.list()] diff --git a/tests/conftest.py b/tests/conftest.py index dd0fc25..5694471 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,7 +29,7 @@ import tempfile import weakref from pathlib import Path from types import SimpleNamespace -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock import pytest @@ -281,6 +281,10 @@ def make_hass() -> tuple: config=MagicMock(), config_entries=SimpleNamespace(async_reload=None), ) + # hass.auth mirrors the real surface the integration reads: an + # async_get_users list the tests can repoint at will (the setup + # path resolves HA owner accounts as the last-resort admin seed). + hass.auth = SimpleNamespace(async_get_users=AsyncMock(return_value=[])) def _cleanup_config_dir(): import shutil diff --git a/tests/test_admin_allowlist.py b/tests/test_admin_allowlist.py index 4ddbf28..c371815 100644 --- a/tests/test_admin_allowlist.py +++ b/tests/test_admin_allowlist.py @@ -302,3 +302,73 @@ async def test_setup_seed_prefers_options_over_entry_data( finally: await database.close() assert await async_unload_entry(hass, entry) is True + + +async def test_setup_owner_fallback_seeds_owner_accounts( + hass, make_entry +) -> None: + """Last-resort seed: with no persisted copy and no flow-context + user, HA owner accounts are seeded so first setup is never + ownerless (the real flow may lose the context; owners can already + do everything in HA).""" + from unittest.mock import AsyncMock + + hass.auth.async_get_users = AsyncMock( + return_value=[ + SimpleNamespace(id="owner-1", is_owner=True), + SimpleNamespace(id="member-1", is_owner=False), + SimpleNamespace(id="owner-2", is_owner=True), + ] + ) + entry = await _setup_entry(hass, make_entry(), hass.registry) + database = entry.runtime_data.database + try: + assert sorted(await list_admin_ids(database)) == ["owner-1", "owner-2"] + finally: + await database.close() + assert await async_unload_entry(hass, entry) is True + + +async def test_setup_owner_fallback_not_used_when_context_present( + hass, make_entry +) -> None: + """The context user (flow caller) wins over the owner fallback: + a non-owner parent configuring the integration becomes the admin.""" + from unittest.mock import AsyncMock + + hass.auth.async_get_users = AsyncMock( + return_value=[SimpleNamespace(id="owner-1", is_owner=True)] + ) + entry = await _setup_entry( + hass, + make_entry(context={"user_id": "parent-configured"}), + hass.registry, + ) + database = entry.runtime_data.database + try: + assert await list_admin_ids(database) == ["parent-configured"] + finally: + await database.close() + assert await async_unload_entry(hass, entry) is True + + +def test_seed_owner_ids_argument(tmp_path) -> None: + async def _body(database): + seeded = await seed_setup_admin( + database, + stored_admin_ids=None, + context_user_id=None, + owner_ids=["owner-1"], + ) + assert seeded == ["owner-1"] + # Malformed owner lists are rejected like any other candidate. + with pytest.raises(ValueError): + await seed_setup_admin( + database, + stored_admin_ids=None, + context_user_id=None, + owner_ids=[7], + ) + return seeded + + _with_db(tmp_path, "seed-owner-arg.db")(_body) From 7ebd893823b48a0a8546513745c6689f92fdec55 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:23:32 -0400 Subject: [PATCH 15/18] Test fix: malformed owner_ids validated before the seed's early return The seed validates candidates only when the allowlist is empty; the test's successful first seed masked the malformed case. The rejection now runs first. Suite: 908 passed. --- tests/test_admin_allowlist.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/test_admin_allowlist.py b/tests/test_admin_allowlist.py index c371815..7b4def1 100644 --- a/tests/test_admin_allowlist.py +++ b/tests/test_admin_allowlist.py @@ -354,14 +354,9 @@ async def test_setup_owner_fallback_not_used_when_context_present( def test_seed_owner_ids_argument(tmp_path) -> None: async def _body(database): - seeded = await seed_setup_admin( - database, - stored_admin_ids=None, - context_user_id=None, - owner_ids=["owner-1"], - ) - assert seeded == ["owner-1"] - # Malformed owner lists are rejected like any other candidate. + # Malformed owner lists are rejected like any other candidate — + # while the allowlist is still empty, so the seed proceeds far + # enough to validate. with pytest.raises(ValueError): await seed_setup_admin( database, @@ -369,6 +364,14 @@ def test_seed_owner_ids_argument(tmp_path) -> None: context_user_id=None, owner_ids=[7], ) + assert await list_admin_ids(database) == [] + seeded = await seed_setup_admin( + database, + stored_admin_ids=None, + context_user_id=None, + owner_ids=["owner-1"], + ) + assert seeded == ["owner-1"] return seeded _with_db(tmp_path, "seed-owner-arg.db")(_body) From 0e4f890ef93773b2a073b5f6d1381a6f4c4f7cc1 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:26:53 -0400 Subject: [PATCH 16/18] Review fix: reopened-record setup closes the database when seeding fails Review round 5 (Codex gpt-5.6-terra, MEDIUM): the reopened-runtime setup path leaked the just-opened connection if owner lookup or seed_setup_admin raised, so HA's setup retries could accumulate open handles. Seeding on that path is now wrapped symmetrically with the first-setup path: any failure closes the database before re-raising, with a test that fails seeding on a re-setup and asserts the close. Suite: 909 passed. --- custom_components/nestquest/__init__.py | 40 +++++++++++++++---------- tests/test_admin_allowlist.py | 40 +++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index d69e2d5..91a962d 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -270,22 +270,30 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: db = await _async_open_database( hass, await async_get_db_path(hass) ) - stored_admin_ids = ( - entry.options.get(CONF_ADMIN_USER_IDS) - or entry.data.get(CONF_ADMIN_USER_IDS) - ) - context = getattr(entry, "context", None) - context_user_id = ( - context.get("user_id") - if isinstance(context, dict) - else None - ) - await seed_setup_admin( - db, - stored_admin_ids=stored_admin_ids, - context_user_id=context_user_id, - owner_ids=await _async_owner_user_ids(hass), - ) + try: + stored_admin_ids = ( + entry.options.get(CONF_ADMIN_USER_IDS) + or entry.data.get(CONF_ADMIN_USER_IDS) + ) + context = getattr(entry, "context", None) + context_user_id = ( + context.get("user_id") + if isinstance(context, dict) + else None + ) + await seed_setup_admin( + db, + stored_admin_ids=stored_admin_ids, + context_user_id=context_user_id, + owner_ids=await _async_owner_user_ids(hass), + ) + except BaseException: + # A seeding failure must not leak the just-opened + # connection: close it before the error propagates, so + # HA's setup retry starts from a clean handle (the + # first-setup path closes symmetrically). + await db.close() + raise existing.database = db entry.runtime_data = existing return True diff --git a/tests/test_admin_allowlist.py b/tests/test_admin_allowlist.py index 7b4def1..b5d15eb 100644 --- a/tests/test_admin_allowlist.py +++ b/tests/test_admin_allowlist.py @@ -375,3 +375,43 @@ def test_seed_owner_ids_argument(tmp_path) -> None: return seeded _with_db(tmp_path, "seed-owner-arg.db")(_body) + + +async def test_reopened_setup_failure_closes_database( + hass, make_entry, monkeypatch +) -> None: + """A seeding failure on the reopened-record path closes the just- + opened connection before re-raising (no leaked handles across HA's + setup retries).""" + import pytest as pytest_module + from unittest.mock import AsyncMock + + import custom_components.nestquest as nestquest_module + from custom_components.nestquest.db import NestQuestDatabase + + # First: a successful setup to create the runtime record and file. + entry = await _setup_entry(hass, make_entry(), hass.registry) + await async_unload_entry(hass, entry) + + # Now a re-setup whose seeding fails after the reopen. + async def _failing_seed(database, **kwargs): + raise RuntimeError("seed boom") + + monkeypatch.setattr( + nestquest_module, "seed_setup_admin", _failing_seed, raising=True + ) + + close_calls: list[bool] = [] + original_close = NestQuestDatabase.close + + async def _spy_close(self): + close_calls.append(True) + return await original_close(self) + + monkeypatch.setattr(NestQuestDatabase, "close", _spy_close) + + with pytest_module.raises(RuntimeError, match="seed boom"): + await _setup_entry(hass, entry, hass.registry) + # The reopen path opened AND closed a connection; the failure is + # the last thing observed. + assert close_calls[-1] is True From 913d3658b8b30e1c78c7a6caa073c8de6f593faf Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:30:29 -0400 Subject: [PATCH 17/18] Review fix: omitted picker submissions carry the allowlist forward MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 6 (Codex gpt-5.6-terra, MEDIUM): HA replaces entry.options wholesale, so a submission omitting the picker dropped the allowlist copy and left only the stale config-flow copy in entry data — a later database loss would resurrect removed admins. Omitted submissions now carry the current DB allowlist forward on the options copy. The legacy-path test updated to assert the carried copy. Suite: 910 passed. --- custom_components/nestquest/options_flow.py | 7 ++++ tests/test_options_flow.py | 36 +++++++++++++++++++-- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/custom_components/nestquest/options_flow.py b/custom_components/nestquest/options_flow.py index 9ab8214..84ac218 100644 --- a/custom_components/nestquest/options_flow.py +++ b/custom_components/nestquest/options_flow.py @@ -213,6 +213,13 @@ class NestQuestOptionsFlow(config_entries.OptionsFlowWithConfigEntry): } if admin_ids_present: data[CONF_ADMIN_USER_IDS] = admin_ids + elif admin_default: + # HA replaces entry.options wholesale, so an omitted picker + # field must carry the CURRENT allowlist forward: dropping + # it would leave only the stale config-flow copy in entry + # data, and a later database loss would resurrect admins + # the owner has since removed. + data[CONF_ADMIN_USER_IDS] = list(admin_default) return self.async_create_entry(title="NestQuest", data=data) def _current_values(self) -> dict[str, Any]: diff --git a/tests/test_options_flow.py b/tests/test_options_flow.py index 0830b5c..bef04c8 100644 --- a/tests/test_options_flow.py +++ b/tests/test_options_flow.py @@ -554,8 +554,9 @@ def test_options_flow_unknown_selection_refused(tmp_path) -> None: def test_options_flow_submit_without_admin_key_keeps_allowlist(tmp_path) -> None: - """Raw callers omitting the key (pre-picker payloads) leave the - allowlist untouched and produce the legacy three-key entry.""" + """Raw callers omitting the key (pre-picker payloads) do not + touch the allowlist; the CURRENT allowlist is carried forward on + the options copy so no removed admin can be resurrected later.""" database = _open_allowlist_db(tmp_path, "picker-legacy.db") try: from custom_components.nestquest.admin_allowlist import ( @@ -572,7 +573,7 @@ def test_options_flow_submit_without_admin_key_keeps_allowlist(tmp_path) -> None ) result = _run(flow.async_step_init(dict(VALID_INPUT))) assert result["type"] == "create_entry" - assert CONF_ADMIN_USER_IDS not in result["data"] + assert result["data"][CONF_ADMIN_USER_IDS] == ["u1"] assert await_gather(list_admin_ids(database)) == ["u1"] finally: _run(database.close()) @@ -605,3 +606,32 @@ def test_options_flow_duplicate_selection_refused(tmp_path) -> None: assert await_gather(list_admin_ids(database)) == ["u1"] finally: _run(database.close()) + + +def test_options_flow_omitted_picker_carries_allowlist_forward( + tmp_path, +) -> None: + """An omitted picker field must not drop the allowlist copy from + the returned options: HA replaces entry.options wholesale, and a + later database loss must not resurrect removed admins from the + stale config-flow copy in entry.data.""" + database = _open_allowlist_db(tmp_path, "picker-carry.db") + try: + from custom_components.nestquest.admin_allowlist import ( + set_admin_ids, + ) + + _run(set_admin_ids(database, ["u2"])) + entry = _make_entry() + flow = _make_flow( + entry, + users=[_user("u1", "Joshua"), _user("u2", "Sam")], + database=database, + ) + result = _run(flow.async_step_init(dict(VALID_INPUT))) + assert result["type"] == "create_entry" + # The current allowlist rides along on the legacy three-key path. + assert result["data"][CONF_ADMIN_USER_IDS] == ["u2"] + assert result["data"][CONF_HORIZON_DAYS] == VALID_INPUT[CONF_HORIZON_DAYS] + finally: + _run(database.close()) From 7cde256e2afaf73d5238ad1236f25a6b65cabe5a Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 17:35:13 -0400 Subject: [PATCH 18/18] Fail-closed is_admin resolver (Feature 03, task 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin_allowlist.is_admin: True only for an allowlisted HA user id; False for non-allowlisted, unknown, empty-string, untrimmed variants, None, non-strings (bools, lists), and an empty allowlist (nobody is admin rather than everybody). Never raises for malformed input — malformed means not admin; DB errors propagate for the Feature 09 gate to handle. The last-admin removal refusal (set_admin_ids) is re-asserted as keeping the verdict sound. Tests: full verdict matrix, empty-allowlist fail-closed, refusal persistence. Suite: 912 passed. --- .../nestquest/admin_allowlist.py | 17 ++++++ tests/test_admin_allowlist.py | 54 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/custom_components/nestquest/admin_allowlist.py b/custom_components/nestquest/admin_allowlist.py index a672d2e..beed12c 100644 --- a/custom_components/nestquest/admin_allowlist.py +++ b/custom_components/nestquest/admin_allowlist.py @@ -91,6 +91,23 @@ async def set_admin_ids( await dao.add(user_id, _now_stamp()) +async def is_admin( + database: NestQuestDatabase, user_id: object +) -> bool: + """Resolve an HA user ID against the allowlist — FAILS CLOSED. + + True only for an allowlisted user id. Everything else is False: + a non-allowlisted id, an unknown id, a None or non-string value + (a service call with no user context), and an empty allowlist + (nobody is admin rather than everybody). The Feature 09 gate + wraps every admin-only service with this verdict; it never raises + for malformed input — malformed means not admin. + """ + if not isinstance(user_id, str) or not user_id: + return False + return await AdminUsersDao(database).exists(user_id) + + async def seed_setup_admin( database: NestQuestDatabase, *, diff --git a/tests/test_admin_allowlist.py b/tests/test_admin_allowlist.py index b5d15eb..5640d2d 100644 --- a/tests/test_admin_allowlist.py +++ b/tests/test_admin_allowlist.py @@ -415,3 +415,57 @@ async def test_reopened_setup_failure_closes_database( # The reopen path opened AND closed a connection; the failure is # the last thing observed. assert close_calls[-1] is True + + +# --------------------------------------------------------------------------- +# is_admin: the fail-closed resolver (Feature 09's verdict function) +# --------------------------------------------------------------------------- + + +def test_is_admin_verdict_matrix(tmp_path) -> None: + async def _body(database): + from custom_components.nestquest.admin_allowlist import is_admin + + await set_admin_ids(database, ["user-owner"]) + assert await is_admin(database, "user-owner") is True + assert await is_admin(database, "user-other") is False + # Unknown, malformed, and absent identities all fail closed. + assert await is_admin(database, "") is False + assert await is_admin(database, " user-owner ") is False, ( + "ids are stored trimmed; an untrimmed lookup is a different " + "id and must not match" + ) + assert await is_admin(database, None) is False + assert await is_admin(database, 42) is False + assert await is_admin(database, ["user-owner"]) is False + assert await is_admin(database, True) is False + return None + + _with_db(tmp_path, "resolver-matrix.db")(_body) + + +def test_is_admin_empty_allowlist_fails_closed(tmp_path) -> None: + """An empty allowlist returns False for EVERYONE, not True.""" + async def _body(database): + from custom_components.nestquest.admin_allowlist import is_admin + + for candidate in ("user-owner", "", None, 1): + assert await is_admin(database, candidate) is False + return None + + _with_db(tmp_path, "resolver-empty.db")(_body) + + +def test_is_admin_false_after_last_admin_removal_attempt(tmp_path) -> None: + """The refusal keeps the verdict sound: a refused removal leaves + the admin in place, so is_admin still resolves True.""" + async def _body(database): + from custom_components.nestquest.admin_allowlist import is_admin + + await set_admin_ids(database, ["user-owner"]) + with pytest.raises(ValueError, match="at least one admin"): + await set_admin_ids(database, []) + assert await is_admin(database, "user-owner") is True + return None + + _with_db(tmp_path, "resolver-refusal.db")(_body)