From 920be7cac1215c36d76fd4bb712e3ed5985aedd1 Mon Sep 17 00:00:00 2001 From: ArticOdin Date: Sun, 24 May 2026 16:32:39 +1000 Subject: [PATCH 1/5] test(silver): enumerate handlers + per-handler raise check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior assertion src.count("raise HomeAssistantError(") >= 3 hard-coded the handler count at the time of writing. When handle_reset_today was added in beta.8 without a raise, the test stayed green because the count was still ≥ 3 — the new handler just didn't add to it. Gemini caught the compliance gap on PR #152 review. Test now enumerates handlers via regex on async def handle_, checks each handler's body contains raise HomeAssistantError(, and reports specific missing handlers in the assertion message. Threshold auto-scales: adding a new handler without the raise breaks the test. Also asserts >= 4 handlers exist so a future refactor that accidentally removes one trips the test. No manifest bump — test-only change, no HACS deploy needed. Full test suite: 1140 passing. --- tests/test_silver_checklist.py | 54 ++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/tests/test_silver_checklist.py b/tests/test_silver_checklist.py index 7d69501..9326020 100644 --- a/tests/test_silver_checklist.py +++ b/tests/test_silver_checklist.py @@ -161,12 +161,60 @@ def test_init_imports_home_assistant_error(self): ) assert "ServiceValidationError" in src - def test_handlers_raise_home_assistant_error_on_missing_coordinator(self): + def test_every_service_handler_raises_home_assistant_error(self): + """Silver action-exceptions rule: every service handler must raise + HomeAssistantError on unrecoverable conditions (missing coordinator, + no entries, etc). Previous version of this test counted total + ``raise HomeAssistantError(`` occurrences and asserted ``>= 3``, + which was the handler count at the time. When ``handle_reset_today`` + was added in beta.8 it silently skipped the raise — the test stayed + green because the count was still ≥ 3 (the new handler didn't add + to it). Gemini caught the compliance gap on PR #152. + + Fix: enumerate handlers dynamically and check each one ends with + ``raise HomeAssistantError(`` somewhere in its body. Threshold + auto-scales with handler count so adding a new handler without + the raise breaks the test. + """ + import re as _re src = ( REPO / "custom_components" / "pricehawk" / "__init__.py" ).read_text() - # At least one raise per handler — count must match the three handlers. - assert src.count("raise HomeAssistantError(") >= 3 + + # Each handler is an ``async def handle_(call: object) -> None`` + # nested inside async_setup_entry. Split the file at handler defs + # and check the body of each contains at least one raise. + handler_starts = [ + m for m in _re.finditer( + r"^ async def handle_(\w+)\(call: object\) -> None:", + src, _re.MULTILINE, + ) + ] + assert len(handler_starts) >= 4, ( + f"Expected at least 4 service handlers in __init__.py, " + f"found {len(handler_starts)}. Update this test if handlers " + f"were intentionally removed." + ) + + missing_raises: list[str] = [] + for i, start in enumerate(handler_starts): + handler_name = start.group(1) + body_start = start.end() + body_end = ( + handler_starts[i + 1].start() + if i + 1 < len(handler_starts) + else len(src) + ) + handler_body = src[body_start:body_end] + if "raise HomeAssistantError(" not in handler_body: + missing_raises.append(handler_name) + + assert not missing_raises, ( + f"Silver action-exceptions: these handlers don't raise " + f"HomeAssistantError anywhere in their body: {missing_raises}. " + f"Every service handler must raise on unrecoverable conditions " + f"(missing coordinator, no entries, etc)." + ) def test_handlers_raise_service_validation_error_on_bad_input(self): src = ( From ff51dbb7b63d00645d75017dd56f0b9c772c0dde Mon Sep 17 00:00:00 2001 From: ArticOdin Date: Sun, 24 May 2026 16:37:05 +1000 Subject: [PATCH 2/5] test(silver): rewrite handler-raise check with ast (gemini PR #154 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini flagged the regex-based approach as fragile: the last handler's sliced 'body' extended to EOF, swallowing post-handler text (service registrations, future module-level code) into false positives. Rewrote with ast: parse the file, walk to AsyncFunctionDef nodes named handle_*, then per-handler walk its body checking for raise HomeAssistantError(...). Nested AsyncFunctionDef nodes are skipped (they're their own scope, validated when they're top-level). Self-test: ast finds 4 handlers (analyze_csv, backfill, rank_alternatives, reset_today) — same as ground truth. --- tests/test_silver_checklist.py | 97 ++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/tests/test_silver_checklist.py b/tests/test_silver_checklist.py index 9326020..6cfdc60 100644 --- a/tests/test_silver_checklist.py +++ b/tests/test_silver_checklist.py @@ -164,56 +164,71 @@ def test_init_imports_home_assistant_error(self): def test_every_service_handler_raises_home_assistant_error(self): """Silver action-exceptions rule: every service handler must raise HomeAssistantError on unrecoverable conditions (missing coordinator, - no entries, etc). Previous version of this test counted total - ``raise HomeAssistantError(`` occurrences and asserted ``>= 3``, - which was the handler count at the time. When ``handle_reset_today`` - was added in beta.8 it silently skipped the raise — the test stayed - green because the count was still ≥ 3 (the new handler didn't add - to it). Gemini caught the compliance gap on PR #152. - - Fix: enumerate handlers dynamically and check each one ends with - ``raise HomeAssistantError(`` somewhere in its body. Threshold - auto-scales with handler count so adding a new handler without - the raise breaks the test. + no entries, etc). + + Prior versions of this test counted total occurrences with a + hard-coded threshold (``>= 3``) — when handle_reset_today was added + in beta.8 without a raise, the count was still 3 and the test + stayed green. Gemini caught the compliance gap on PR #152. + + First rewrite used regex-sliced "function bodies", which gemini + flagged on PR #154 as fragile: the last handler's "body" extended + to EOF, swallowing any post-handler ``raise HomeAssistantError(`` + text (e.g. in registration calls or docstrings) into a false + positive. + + Final version uses ``ast`` to walk real function bodies. Per + handler: find every ``raise HomeAssistantError(...)`` node inside + the function definition. Threshold auto-scales with handler count. """ - import re as _re + import ast src = ( REPO / "custom_components" / "pricehawk" / "__init__.py" ).read_text() - - # Each handler is an ``async def handle_(call: object) -> None`` - # nested inside async_setup_entry. Split the file at handler defs - # and check the body of each contains at least one raise. - handler_starts = [ - m for m in _re.finditer( - r"^ async def handle_(\w+)\(call: object\) -> None:", - src, _re.MULTILINE, - ) + tree = ast.parse(src) + + def _iter_async_funcs(node): + """Yield AsyncFunctionDef nodes recursively (handlers may be + nested inside async_setup_entry, etc).""" + for child in ast.walk(node): + if isinstance(child, ast.AsyncFunctionDef): + yield child + + def _body_has_raise_hae(func: ast.AsyncFunctionDef) -> bool: + """Walk only this function's body (not nested defs) and check + for ``raise HomeAssistantError(...)``.""" + for stmt in func.body: + for child in ast.walk(stmt): + if isinstance(child, ast.AsyncFunctionDef): + # Don't descend into nested defs — they're their + # own scope and validated separately. + continue + if not isinstance(child, ast.Raise): + continue + exc = child.exc + if isinstance(exc, ast.Call) and isinstance( + exc.func, ast.Name + ) and exc.func.id == "HomeAssistantError": + return True + return False + + handlers = [ + f for f in _iter_async_funcs(tree) + if f.name.startswith("handle_") ] - assert len(handler_starts) >= 4, ( + assert len(handlers) >= 4, ( f"Expected at least 4 service handlers in __init__.py, " - f"found {len(handler_starts)}. Update this test if handlers " - f"were intentionally removed." + f"found {len(handlers)}: {[h.name for h in handlers]}." ) - missing_raises: list[str] = [] - for i, start in enumerate(handler_starts): - handler_name = start.group(1) - body_start = start.end() - body_end = ( - handler_starts[i + 1].start() - if i + 1 < len(handler_starts) - else len(src) - ) - handler_body = src[body_start:body_end] - if "raise HomeAssistantError(" not in handler_body: - missing_raises.append(handler_name) - - assert not missing_raises, ( + missing = [ + h.name for h in handlers + if not _body_has_raise_hae(h) + ] + assert not missing, ( f"Silver action-exceptions: these handlers don't raise " - f"HomeAssistantError anywhere in their body: {missing_raises}. " - f"Every service handler must raise on unrecoverable conditions " - f"(missing coordinator, no entries, etc)." + f"HomeAssistantError anywhere in their body: {missing}. " + f"Every service handler must raise on unrecoverable conditions." ) def test_handlers_raise_service_validation_error_on_bad_input(self): From f832262574afe7bd269b1369ae82a5b60eaa4d59 Mon Sep 17 00:00:00 2001 From: ArticOdin Date: Sun, 24 May 2026 16:40:59 +1000 Subject: [PATCH 3/5] test(silver): prune nested defs with manual DFS (gemini PR #154 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ast.walk is a flat generator — yields all descendants. My continue on encountered AsyncFunctionDef only skipped that node, not its subtree, so a raise HomeAssistantError inside a nested helper would still satisfy the check for the outer handler. Replaced with manual stack-based DFS using ast.iter_child_nodes. Encountering a FunctionDef/AsyncFunctionDef now skips the entire subtree, not just the def node. Self-verified: synthesised a handler with nested helper that raises HAE, check correctly returns False (nested doesn't count for outer). Per gemini suggestion on PR #154 inline review. --- tests/test_silver_checklist.py | 37 +++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/tests/test_silver_checklist.py b/tests/test_silver_checklist.py index 6cfdc60..ccaf64f 100644 --- a/tests/test_silver_checklist.py +++ b/tests/test_silver_checklist.py @@ -196,20 +196,33 @@ def _iter_async_funcs(node): def _body_has_raise_hae(func: ast.AsyncFunctionDef) -> bool: """Walk only this function's body (not nested defs) and check - for ``raise HomeAssistantError(...)``.""" + for ``raise HomeAssistantError(...)``. + + Gemini caught on PR #154 that ``ast.walk`` is a flat generator + — it yields every node in the subtree regardless. ``continue`` + on an encountered nested FunctionDef/AsyncFunctionDef only + skipped THAT node, not its children, so a ``raise + HomeAssistantError(...)`` inside a nested helper would still + satisfy the check for the outer handler. Manual DFS with + ``ast.iter_child_nodes`` properly prunes the subtree. + """ for stmt in func.body: - for child in ast.walk(stmt): - if isinstance(child, ast.AsyncFunctionDef): - # Don't descend into nested defs — they're their - # own scope and validated separately. + todo: list[ast.AST] = [stmt] + while todo: + node = todo.pop() + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + # Skip the entire nested-scope subtree — they're + # validated when they're top-level themselves. continue - if not isinstance(child, ast.Raise): - continue - exc = child.exc - if isinstance(exc, ast.Call) and isinstance( - exc.func, ast.Name - ) and exc.func.id == "HomeAssistantError": - return True + if isinstance(node, ast.Raise): + exc = node.exc + if ( + isinstance(exc, ast.Call) + and isinstance(exc.func, ast.Name) + and exc.func.id == "HomeAssistantError" + ): + return True + todo.extend(ast.iter_child_nodes(node)) return False handlers = [ From e0f3e68e7cc87c996b016802ffe8df2bb59bf4de Mon Sep 17 00:00:00 2001 From: ArticOdin Date: Sun, 24 May 2026 16:46:08 +1000 Subject: [PATCH 4/5] test(silver): accept ServiceValidationError + prune ClassDef + sync defs (gemini #154 review-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini's third pass on PR #154 flagged three remaining gaps: 1. Silver action-exceptions accepts EITHER HomeAssistantError OR ServiceValidationError per HA docs. The check accepted only HAE, so a future handler that legitimately raises only SVE for user- input validation would fail the test. Now accepts both via a frozenset. 2. ClassDef wasn't in the prune list. A handler with an inline helper class whose method raises HAE/SVE would wrongly satisfy the check for the outer handler. Added to the skip set. 3. Sync function handlers (def handle_x, not async def) were missed entirely. The iter helper only yielded AsyncFunctionDef. Now yields both — a future non-async handler still gets validated. Self-verified each: nested-class raise no longer counts toward the outer handler (negative test in the commit msg below). Negative test snippet: async def handle_x(call): class Helper: def fail(self): raise HomeAssistantError('inner') pass DFS result: False ✓ (correctly does NOT count the class method's raise) --- tests/test_silver_checklist.py | 61 +++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/tests/test_silver_checklist.py b/tests/test_silver_checklist.py index ccaf64f..4938b66 100644 --- a/tests/test_silver_checklist.py +++ b/tests/test_silver_checklist.py @@ -187,48 +187,60 @@ def test_every_service_handler_raises_home_assistant_error(self): ).read_text() tree = ast.parse(src) - def _iter_async_funcs(node): - """Yield AsyncFunctionDef nodes recursively (handlers may be - nested inside async_setup_entry, etc).""" + # Silver's action-exceptions rule accepts either HomeAssistantError + # or ServiceValidationError per HA docs; the latter is the typed + # subclass for "user supplied bad input". Per gemini follow-up on + # PR #154 — backfill_history and rank_alternatives use SVE for + # type-cast failures, and a future handler that ONLY raises SVE + # would still satisfy the rule. + _ACTION_EXCEPTIONS = frozenset( + {"HomeAssistantError", "ServiceValidationError"} + ) + + def _iter_handler_funcs(node): + """Yield function/async-function defs whose name starts with + ``handle_``. Async is the norm, but accept sync too — a future + non-async handler shouldn't silently skip validation.""" for child in ast.walk(node): - if isinstance(child, ast.AsyncFunctionDef): + if isinstance( + child, (ast.FunctionDef, ast.AsyncFunctionDef) + ) and child.name.startswith("handle_"): yield child - def _body_has_raise_hae(func: ast.AsyncFunctionDef) -> bool: - """Walk only this function's body (not nested defs) and check - for ``raise HomeAssistantError(...)``. + def _body_has_action_exception_raise(func) -> bool: + """Walk only this function's body (not nested scopes) and check + for ``raise HomeAssistantError(...)`` or + ``raise ServiceValidationError(...)``. Gemini caught on PR #154 that ``ast.walk`` is a flat generator - — it yields every node in the subtree regardless. ``continue`` - on an encountered nested FunctionDef/AsyncFunctionDef only - skipped THAT node, not its children, so a ``raise - HomeAssistantError(...)`` inside a nested helper would still - satisfy the check for the outer handler. Manual DFS with - ``ast.iter_child_nodes`` properly prunes the subtree. + — it yields every node in the subtree regardless of any + ``continue``. Manual DFS with ``ast.iter_child_nodes`` properly + prunes nested scopes (FunctionDef, AsyncFunctionDef, ClassDef + — the latter to avoid a handler-internal helper class's method + wrongly counting toward the outer handler). """ for stmt in func.body: todo: list[ast.AST] = [stmt] while todo: node = todo.pop() - if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): - # Skip the entire nested-scope subtree — they're - # validated when they're top-level themselves. + if isinstance( + node, + (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef), + ): + # Skip the entire nested-scope subtree. continue if isinstance(node, ast.Raise): exc = node.exc if ( isinstance(exc, ast.Call) and isinstance(exc.func, ast.Name) - and exc.func.id == "HomeAssistantError" + and exc.func.id in _ACTION_EXCEPTIONS ): return True todo.extend(ast.iter_child_nodes(node)) return False - handlers = [ - f for f in _iter_async_funcs(tree) - if f.name.startswith("handle_") - ] + handlers = list(_iter_handler_funcs(tree)) assert len(handlers) >= 4, ( f"Expected at least 4 service handlers in __init__.py, " f"found {len(handlers)}: {[h.name for h in handlers]}." @@ -236,12 +248,13 @@ def _body_has_raise_hae(func: ast.AsyncFunctionDef) -> bool: missing = [ h.name for h in handlers - if not _body_has_raise_hae(h) + if not _body_has_action_exception_raise(h) ] assert not missing, ( f"Silver action-exceptions: these handlers don't raise " - f"HomeAssistantError anywhere in their body: {missing}. " - f"Every service handler must raise on unrecoverable conditions." + f"HomeAssistantError or ServiceValidationError anywhere in " + f"their (top-level) body: {missing}. Every service handler " + f"must raise on unrecoverable conditions." ) def test_handlers_raise_service_validation_error_on_bad_input(self): From 59bc73d7408fc12092e8a39c0a34e09281db60fe Mon Sep 17 00:00:00 2001 From: ArticOdin Date: Sun, 24 May 2026 16:49:20 +1000 Subject: [PATCH 5/5] test(silver): also detect bare-class raises (gemini #154 review-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit raise HomeAssistantError (no parens) is valid Python and used in some HA integrations. Prior code only matched ast.Call wrapping ast.Name — the bare form was an ast.Name directly on node.exc. Per gemini suggestion: unify both forms by unwrapping ast.Call to its .func first, then doing the single ast.Name check at the end. Self-verified: bare raise HomeAssistantError correctly detected. --- tests/test_silver_checklist.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/test_silver_checklist.py b/tests/test_silver_checklist.py index 4938b66..3a1bef4 100644 --- a/tests/test_silver_checklist.py +++ b/tests/test_silver_checklist.py @@ -230,11 +230,17 @@ def _body_has_action_exception_raise(func) -> bool: # Skip the entire nested-scope subtree. continue if isinstance(node, ast.Raise): - exc = node.exc + # Handle both ``raise X(...)`` (ast.Call wrapping + # ast.Name) and the bare-class form ``raise X`` + # (ast.Name directly). Gemini caught the bare-form + # gap on PR #154 — it's valid Python and used in + # some HA integrations. + exc: ast.AST | None = node.exc + if isinstance(exc, ast.Call): + exc = exc.func if ( - isinstance(exc, ast.Call) - and isinstance(exc.func, ast.Name) - and exc.func.id in _ACTION_EXCEPTIONS + isinstance(exc, ast.Name) + and exc.id in _ACTION_EXCEPTIONS ): return True todo.extend(ast.iter_child_nodes(node))