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.
This commit is contained in:
Joshua King
2026-09-15 16:35:18 -04:00
parent 5c74034f81
commit cfe1609df2
2 changed files with 23 additions and 1 deletions
+8 -1
View File
@@ -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
+15
View File
@@ -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
# ---------------------------------------------------------------------------