From 7a585afb9e16fdbb0b0b1c1134322e1fabdde1ff Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 16:41:14 -0400 Subject: [PATCH 1/2] 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 2/2] 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]