Skip to content

Commit 0bb3fa0

Browse files
swissmoclaude
andauthored
feat(bulk): add deterministic structural selectors (#2246)
* feat(bulk): add deterministic structural selectors * fix(bulk): address selector review feedback * test(bulk): set selector helpers initially off * test(bulk): use MCP assertions in selector setup * fix(bulk): address second round of selector review feedback Codex's re-review of the structural-selector feature (PR #2246) surfaced four remaining gaps: - bulk_selector: a cross-domain aggregate (e.g. group.living_room_lights) assigned to a selected area was excluded from root candidates before membership expansion, so its ungrouped target-domain members were never discovered. - bulk_selector: a device-registry entry missing `id` was silently skipped by the strict visibility resolver, letting a device-inherited exclusion drop out of the hidden set instead of failing closed (mirrors the per-entry validation in visibility/enforcement.py). - policy/middleware: concurrent identical selector calls shared one pending approval via find_or_create, so a single approval click could release both waiters and dispatch the selector twice. - policy/evaluator: the selector fail-safe gated every ha_bulk_control selector call whenever any rule named the tool, even when that rule's predicates were fully selector-inspectable and simply didn't match — defeating a deliberately conditional rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): detect wildcard rule paths that reach args.operations CodeRabbit caught a gap in the previous fail-safe fix: it only checked for the literal "args.operations" prefix, so a rule written with a leading wildcard (e.g. "args.*.*.entity_id") could match explicit operations-mode calls but silently dodge the selector-mode fail-safe, since its path string never starts with "args.operations" — a real approval bypass for wildcard-authored rules. iter_path_values fans a leading "*" out over every top-level key, so it reaches "operations" exactly as readily as a literal reference does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): scope the wildcard operations fail-safe to list-reaching paths The previous fix (ac2b5bf) over-corrected: it treated ANY leading wildcard as operations-sensitive, but a wildcard followed by a literal segment (e.g. "args.*.domain") can only ever land on a dict value (like selector) -- walk() requires isinstance(cur, dict) for a literal head, so it can never reach the operations LIST. That regressed a legitimate conditional rule (e.g. selector.domain=="lock") back into an unconditional gate for every selector call. Only a second wildcard (args.*.*...) or no further segment (bare args.*, which yields the operations list itself) can actually traverse into operations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): address third round of selector review feedback A comprehensive review flagged two blocking issues plus a long list of correctness, error-routing, and coverage gaps in the structural-selector feature. Fixed: Blocking: - Root admission for aggregate expansion was not domain-constrained: a scene assigned to a selected area was admitted as a root and expanded, pulling entities from anywhere in the house into the dispatch (HA's scene.entity_id attribute lists controlled targets, not structural members). Scenes are now excluded from aggregate-root admission on architectural grounds (not an integration-identity heuristic). - A dynamic selector call's approval could be consumed by a DIFFERENT invocation than the one that created it -- via a race (a concurrent call's find() observing "approved" before the original waiter resumes) or a later identical call reusing a stale approval within approval_ttl_minutes, dispatching against re-resolved topology. Dynamic entries are now bound to their creating invocation only. Also fixed: - A wildcard rule path with a literal tail (args.*.domain) was over-classified as needing resolved-operations data by the previous fix, regressing legitimate conditional selector rules back into an unconditional gate. Narrowed to paths that can actually reach the operations list. - Stringified JSON arguments (Claude Desktop stdio sends these) bypassed policy evaluation entirely: PolicyMiddleware reads raw context.message.arguments before the tool's own Pydantic coercion runs, so a rule targeting a nested field silently never matched. Normalized once, before evaluation and hashing. - _load_topology used a bare asyncio.gather; the first exception left the other four awaitables orphaned. Now mirrors tools_areas.py's return_exceptions=True + explicit re-raise pattern. - Registry/visibility infrastructure failures were raised as the same exception class as caller input errors, reaching the agent as VALIDATION_FAILED and inviting a pointless selector rewrite against an HA outage. Split into BulkSelectorInfrastructureError, routed through create_connection_error. - An unknown selector.domain (e.g. "lights") passed validation and resolved to nothing, blaming exclusions/areas that were correct. Domain existence is now checked against loaded states, and the empty-result message distinguishes "no entities of domain X" from "all matches were excluded or hidden". - VALIDATION_FAILED had no DEFAULT_SUGGESTIONS entry, so ~14 selector failure modes returned no actionable guidance. - _parse_bulk_operations lost its security rationale comments (include_input=False; why malformed rows are preserved) during an earlier extraction. Plus: NamedTuple returns instead of bare tuples, BulkSelectorResolution's operations as a computed property (was a separately-tracked list that could desync from resolved_entity_ids), _SELECTOR_KEYS derived from the TypedDict instead of a parallel literal, a logger for the ~14 selector failure modes, dispatch failures now attach the resolution to their error context, dangling-member/cycle errors name their aggregate and parameter, and the "exactly one of operations/selector" and "selector-only parameter" validation messages now name the actual offender instead of a collapsed generic message. Test coverage added: scene exclusion, aggregate exclusion (only leaf exclusion was tested before), unknown domain, domain-not-in-area vs all-excluded-or-hidden, whitespace-padded IDs, orphaned-task-free topology fetch failure, visibility-infrastructure routing, the wildcard over-classification, stringified-argument policy bypass (pure-function and end-to-end), the concurrent-approval race, and a real non-dry-run E2E dispatch that verifies the excluded entity's live state never moves (the only prior E2E test asserted the dry-run preview, never a real dispatch or real post-dispatch state). Deliberately deferred (documented in PR comment, not silently dropped): a handful of narrower test-coverage gaps (exact MAX_SELECTOR_ENTITIES=100 boundary, individual selector-only-parameter-rejection cases, malformed JSON body edge case) and a request to expose a hidden-vs-excluded breakdown finer than the current warnings message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: fix test_no_suggestions_when_none for VALIDATION_FAILED's new default The CI Unit Tests job failed after f586be2: this test asserted no "suggestions" key appears when the caller passes none, using a bare ValueError (-> VALIDATION_FAILED) as its example. That code now has a DEFAULT_SUGGESTIONS entry (item 8 of the review), so the assertion no longer holds for that specific code -- the test's actual contract (no suggestions when the caller passes none AND the code has no configured default) is still true, it just needs an example that still has no default. Switched to a message-classified RESOURCE_NOT_FOUND case, which has no DEFAULT_SUGGESTIONS entry. Confirmed via a full local reproduction of CI's exact invocation (pytest tests/src/unit/ -n auto, submodule initialized): this was the only failure attributable to the PR's changes among 10662 passing tests; the remaining ones are pre-existing Windows/POSIX-environment mismatches unrelated to this branch (path semantics, file permissions, locale, strftime formatting). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): surface resolution warnings at top level, fix E2E MCPAssertions usage Two CodeRabbit findings on the latest push: - resolution.summary()'s warnings list was left nested under response["resolution"]["warnings"] on both the dry-run and dispatch return paths, contradicting AGENTS.md's "warnings is always a top-level list[str], never nested" contract -- a consumer reading the top-level key never saw the hidden-entity degradation. Extracted _attach_resolution_to_response() (also needed to keep _run_bulk_selector under the C901 complexity threshold) which pops the nested warnings and extends (not overwrites) any warnings bulk_device_control's own response already carries. - The new E2E dispatch test read the excluded entity's post-dispatch state via the raw mcp_client + parse_mcp_result outside the MCPAssertions context, instead of mcp.call_tool_success() inside it -- inconsistent with this PR's own earlier fix (c62d9e4) and the repo's coding guideline for success-path tool calls in E2E tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): address CodeRabbit nitpicks (private import, shared state, exception tuple) - policy/evaluator.py imported tools/util_helpers.py's underscore-prefixed _loads_if_json_container_str directly. Renamed it to the public loads_if_json_container_str (evaluator.py already reuses it outside its original BeforeValidator role) instead of reaching into another module's private internals. - BulkSelectorResolution.operations built every row via {**self._operation_common}, which only shallow-copies: all rows shared the SAME "parameters" dict object, so an in-place mutation of one row's parameters would silently leak into every other row. Each row now gets its own copy. - The selection and exclusion sides of resolve_bulk_selector shared one expanded_groups accumulator, so an aggregate referenced only via exclude_entity_ids (plus any of its own nested sub-groups) showed up in expanded_group_ids as if it had fed the selection. Split into two accumulators (selection-side reported, exclusion-side not); the memoization cache stays shared since an entity referenced by both sides should still only be expanded once. - tools_service.py caught (ValueError, BulkSelectorValidationError) together; the latter subclasses the former, so the tuple was redundant. Two inline findings from the same review pass (warnings surfacing, MCPAssertions usage in the E2E test) were already fixed in c3acc51; verified against current code and left alone. Not applied: a nitpick suggesting dynamic selector retries supersede or share a bounded set of pending-approval rows. Superseding an existing pending entry on retry cannot be distinguished from two genuinely concurrent calls each still legitimately waiting (both look identical to the queue: a new call arriving while an older entry for the same tool_name+args_hash exists) -- superseding would silently invalidate a live approval opportunity for the concurrent case, reintroducing a correctness bug worse than the UX nitpick it fixes. The creator-only binding from f586be2 is kept as the safe, minimal fix; a real solution needs invocation-liveness tracking or is better solved in the settings UI (only surface the newest row per selector as actionable), out of scope for this backend change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): re-raise ToolError from bulk_device_control unchanged The dispatch try/except added around bulk_device_control (f586be2, to attach the resolution to the error context on failure) caught Exception broadly, which also catches a ToolError bulk_device_control raises itself -- e.g. its own "every operation failed validation" case (device_control.py uses raise_tool_error extensively). Without a guard, exception_to_structured_error re-classifies that already-structured ToolError: none of its type-based cases match ToolError, and its JSON message string doesn't match any of the message-based patterns either, so it falls through to a generic INTERNAL_ERROR -- discarding the real code, message, and suggestions bulk_device_control had already produced. Added the except ToolError: raise guard AGENTS.md documents for exactly this situation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): close remaining selector review gaps Hardens the deterministic-selector feature against the last review pass: redact lock/alarm codes from BulkSelectorResolution's repr, use keyword construction/attribute access instead of positional splats/unpacks on _Topology and _ValidatedSelector (the NamedTuples exist specifically to prevent that), thread visibility-config warnings and log every failed topology fetch instead of the first only, guard the policy layer's recursive stringified-container normalizer against RecursionError, and give both the selector-only-parameter and infrastructure-error messages an actual next step instead of just naming the problem. Restores the operations-mode error contract (partial-batch reporting, all-fail closes the call) and documents MAX_SELECTOR_ENTITIES in the tool docstring. Also closes the test gaps the review flagged: the "*" wildcard arm of both fail-safes, every _selector_only_parameter_offender branch, the registry_hidden (hidden_by) filter, both _registry_rows failure shapes, dangling-member attribution, the exact MAX_SELECTOR_ENTITIES boundary, and a literal-payload assertion for dispatch — plus docstrings on the review's own touched-and-undocumented functions (evaluate, on_call_tool, _raise_pending_error) to close out the docstring-coverage gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): avoid implicit string concatenation inside suggestion lists CodeQL's py/implicit-string-concatenation-in-list flagged 7 wrapped suggestion strings across middleware.py and tools_service.py: each is a single sentence split across lines with no operator between the literals, the same shape a genuinely missing comma would produce. Switches to the explicit `+` already used for this exact case at tools_service.py's ws_command suggestions (~line 961) instead of relying on implicit adjacent-literal concatenation. No message text changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): reject operations-mode batches that mix a group with its own members Live-tested and root-caused (2026-08-23): "turn off the basement except the staircase light" via ha_bulk_control's operations mode correctly omitted the excluded entity from the request, but including the Hue Room group entity alongside its other members still cascaded the action to the excluded one -- operations mode has no exclude_entity_ids to express that intent, and the omission alone never protected it. Adds a pre-dispatch group-safety check: operations mode now fails the whole batch closed (nothing dispatched) whenever it targets a group/aggregate entity together with one or more of its own individual members, using the same generic is_group/member detection already proven correct for this data shape. Fails closed on its own states-fetch failure too, and preserves a ToolError raised during that fetch unchanged (matching the existing dispatch- block guard elsewhere in this file). Selector mode's exclude_entity_ids already handles this correctly and is unaffected; a new regression test pins that against the exact real-world entity shape from the live test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): make selector mode actually reachable after a group-safety rejection Live-tested (2026-08-23): the group-safety rejection correctly blocked the dangerous batch, but the calling model then failed 6+ times trying to recover via selector mode and gave up entirely -- a net worse outcome than before, even though the dangerous action was correctly prevented. Root cause was two separate, pre-existing gaps the model hit hard once the operations-mode escape hatch was no longer available: 1. It put exclude_entity_ids as a top-level argument instead of nested inside selector, and the rejection message only said "use selector mode with exclude_entity_ids" with no worked example to correct that. 2. It passed area display names (from ha_search's friendly area_names, and a floor's display name) as area_ids, which must be exact HA registry IDs -- the "unknown area_ids" error never said so, or how to find the real ones. Both error messages now show a concrete, correctly-shaped example and point to ha_list_floors_areas (the existing tool for exact area_id/floor_id lookup) instead of just naming the problem. Message-only changes, no behavior change to the group-safety check itself -- it continues to block the exact batch shape confirmed live to cascade into an excluded entity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): detect nested group conflicts and skip the states fetch for single-entity batches Two nitpicks against the group-safety check, both verified against current code: 1. _reject_operations_group_member_conflicts always fetched states before checking whether the batch even had 2+ distinct entities, even though a single-entity batch can never contain a group/member conflict. Moved the entity_ids computation and the size check ahead of the fetch, so the overwhelming majority of operations-mode calls -- which never touch a group at all -- no longer pay for a states round-trip they can't use. 2. _find_group_member_conflicts only checked each targeted entity's DIRECT members, missing a conflict reachable through a nested aggregate (an outer Zone group containing an inner Room group, with the batch targeting the outer group and a leaf that only belongs to the inner one) -- exactly as unsafe as targeting the inner group and that leaf directly, since the outer group's own service call still cascades through the inner one. Added a small cycle-safe transitive-membership walk (_expand_membership_transitively) scoped to this check, rather than reusing bulk_selector's _expand_entity, which raises on a dangling member or cycle -- the wrong contract for a best-effort batch scan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): close the round-3 review's 7 blocking gaps in the group-safety gate All 7 blocking items, verified against current code and fixed: 1. bulk_selector.py: the empty-result guard (candidate_roots already subtracts hidden entities) misreported an all-hidden match set as "domain doesn't exist in the area" -- gated on `expanded_leaves` (not just `not selected_leaves`) so the cross-domain-aggregate case it was written for still fires, and the all-hidden case correctly falls through to the excluded-or-hidden message instead. 2+3. rest_client.py: HomeAssistantClient.get_states() silently swallowed a malformed HA response (including _request's own {} JSONDecodeError fallback) to an empty list instead of raising -- the actual fail-OPEN hole in both the operations-mode group gate (an HA hiccup read as "verified, no conflicts") and selector mode (the same swallow made _require_domain_known blame a domain typo for an infrastructure failure). Fixed at the source: get_states() now raises HomeAssistantConnectionError on a non-list response, which both call sites already handle correctly -- their own now-redundant defensive checks were removed rather than left as dead code. 4. tools_service.py: the group-safety gate had no equivalent of bulk_selector._NON_AGGREGATE_ROOT_DOMAINS, so a scene targeted alongside an entity it configures (HA's scene entity_id attribute lists configured targets, not structural members) was flagged with a remedy (selector mode's exclude_entity_ids) that selector mode itself refuses for scenes. Now imports and shares the same constant. 5. middleware.py: a timed-out dynamic-selector pending entry was left in the queue -- a ghost row the settings UI still offered and ApprovalQueue.approve() still accepted, doing nothing because nothing was left holding the pending reference to act on the decision. Now removed before _raise_pending_error runs; its message no longer describes a still-open wait window that had already closed by the time the message is built. 6. tools_service.py: action/parameters/timeout_seconds/validate_first are all declared BulkControlOperation row fields -- the selector-only- parameter message now sends the caller to move the value onto each row instead of discarding their intent by telling them to delete it (only dry_run, which has no per-row equivalent, keeps that remedy). 7. policy/evaluator.py + middleware.py: normalize_stringified_containers silently swallowed its own RecursionError and returned unnormalized args, letting a rule scoped to a nested selector field silently ALLOW instead of gating. Now propagates; middleware.py catches it and fails the call closed with a logged, structured error, matching the corrupt-policy-file precedent already established there. Also fixed (not blocking, but real): the group-conflict message named the harmless redundant rows instead of the members that would actually be silently affected -- now reports transitive_members - entity_ids (falling back to the redundant set only when nothing is unlisted). Plus the smaller items: %r+exc_info over %s for topology-fetch logging (str() on TimeoutError/ConnectionResetError is empty) with per-registry labels; BulkSelectorInfrastructureError's cause is now a StrEnum instead of an open str a typo could silently downgrade; _expand_roots returns its groups instead of mutating a shared out-param (excluded_expanded_groups was write-only); _operation_common stays compare=True (it holds action, so two opposite-action resolutions must not compare equal) with a corrected comment; the resolution's own parameters dict is now copied at construction, not just per dispatch row; _expand_membership_transitively's visited accumulator is keyword-only with an internal default instead of a positional mutable default. 19 new/updated tests covering every fix above, including a parity test over the InfrastructureErrorCause set and a docstring/constant drift guard for MAX_SELECTOR_ENTITIES. Full unit suite (10716 tests) run to confirm the rest_client.py change has no collateral effect on its ~20 other callers -- only the same pre-existing, unrelated Windows-platform failures from earlier this session remain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): CodeQL implicit-concat + a vacuous suggestions-list assertion CodeQL py/implicit-string-concatenation-in-list (2 findings): the item-5 dynamic-approval suggestions list (middleware.py, previous commit) wrapped two multi-line string items with no operator between the literals -- the same shape a missing comma produces. Switched to the explicit "+" already used elsewhere in this file, plus one more instance in the same list-shape (the RecursionError fix's suggestions) that CodeQL hadn't flagged yet but matches the identical pattern. CodeRabbit: test_custom_suggestions_override_the_defaults asserted a default suggestion string was absent from response["error"]["suggestions"] -- but create_error_response only populates the plural "suggestions" key when more than one suggestion is present, and the test passed exactly one, so that key was never set regardless of whether the override worked. The assertion was vacuous: it would pass even if create_connection_error silently ignored custom suggestions entirely. Fixed by passing two custom suggestions and asserting the complete list equals them exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(bulk): add e2e coverage for the group-safety gate against a real HA group Prove the operations-mode group+member rejection against a group.set-created entity (real entity_id/member_entity_ids shape), not just mocked unit-test state, and confirm targeting the group alone still dispatches normally. * fix(bulk): close review round-4's 6 blocking gaps plus 11 smaller findings Blocking: - Stop skipping scene entities in the operations-mode conflict gate: a scene dispatch really does cascade into its configured entities (scene.turn_on), unlike selector mode's aggregate-root question, which is why that skip existed there in the first place. The two now answer their own, different questions instead of sharing one wrong exemption. - Split bulk_selector's empty-result gate into three causes instead of two (empty aggregate / wrong-domain aggregate / hidden), each gated on "nothing in the match set is hidden" -- a hidden entity elsewhere in the same area no longer gets masked by an unrelated visible aggregate expanding to the wrong domain. - Fix the selector-only-parameter worked example rendering a duplicate 'action' key when action is itself the offending parameter. - Cap the group-conflict message's member list at 10 (+N more) instead of enumerating an instance-sized membership list in a fail-closed error whose remedy sentence is the entire value of the response. - Cover both _GroupConflict message branches with tests that would catch the two fields being swapped (previously only the unlisted-members branch was ever exercised). Smaller: - Bind topology fetch-failure labels directly to their named locals instead of a position-zipped tuple, so a future reordered gather() can't silently mislabel which registry failed. - _operation_common needs hash=False, not just compare=True: the frozen dataclass claimed to be hashable (mypy agreed) but raised at the first real hash() call, since a dict has none. - Give the args-too-deeply-nested fail-closed branch its own error code (POLICY_ARGS_TOO_DEEPLY_NESTED) instead of reusing POLICY_LOAD_FAILED, which already means a corrupt tool_policy.json -- a different failure with a different, caller-shaped remedy. - Log the dynamic pending-entry removal at INFO; it was the only silent state transition on the approval queue, and a late Approve click on it used to log as an "attacker probing tokens" WARNING for ordinary use. - Restore actionable suggestions on the group-safety check's own states-fetch failure, lost when the isinstance(states, list) guard was removed -- otherwise the agent sees a raw client string with no hint that the safety check itself is what failed. - Deep-copy operation parameters (both the per-row and resolution-level copies) instead of a shallow dict() that only stops top-level rebinding and still shares nested values like an rgb_color list. - Fix an inverted test docstring describing the guard it tests backwards. - Assert expires_in_seconds is absent from the dynamic pending-error body, not just that its message/suggestions text omits a countdown. - Extend the topology multi-failure test to all five registries instead of two, closing the remaining label-typo blind spot. - E2E test: use safe_call_tool for cleanup (so a cleanup failure can't mask a real assertion failure in finally), and discover real lights dynamically instead of hardcoding demo-platform entity IDs. Skipped: a CodeRabbit suggestion to replace the established MCPAssertions.call_tool_failure helper (used across 13 e2e files) with a manual safe_call_tool call in the new E2E test -- no functional benefit, and goes against the repo's own convention. Also skipped narrowing ServiceTools' client: Any typing (untested seam around a dict-shaped get_states() response) -- reviewer confirmed this is not a live gap, and the fix would mean a broader typing change across ServiceTools, register_service_tools, and registry.py. 10722 unit tests pass (up from 10716 by exactly the 6 new tests added here); the 26 remaining failures are the same pre-existing, Windows-platform-only failures already present before this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(bulk): route the e2e test's light discovery through call_tool_success A bare mcp_client.call_tool() for ha_search meant a real tool failure was indistinguishable from "no lights found" -- both landed on the same pytest.skip(), silently hiding an infrastructure problem instead of failing the test. Folds the search into the test's existing MCPAssertions context so call_tool_success raises loudly on an actual error. * fix(bulk): narrow client: Any to HomeAssistantClient, pin deep-copy and example-value fixes 1. Narrow the untyped client seam the deleted malformed-states test used to protect. ServiceTools.__init__, _reject_operations_group_member_conflicts, register_service_tools, resolve_bulk_selector, and _load_topology now take HomeAssistantClient instead of Any -- so a states-shaped response from the wrong client (e.g. the websocket client's get_states() -> dict[str, Any], vs. the REST client's list[dict[str, Any]]) is a static error again, not a silent fail-open on the group-safety gate. This did cascade one step, as flagged as a possibility: registry.py's ToolsRegistry.__init__(server: Any, ...) was the remaining break in the chain (self.client = server.client inferred Any from it). Closed with a small Protocol (_ServerLike, matching the existing CustomRouteServer pattern in browser_landing.py) instead of importing the concrete HomeAssistantSmartMCPServer class, since server.py's own lazy import of ToolsRegistry exists specifically to avoid that circular import. Narrowing surfaced two real (independent, pre-existing) issues along the way, both fixed: _capture_initial_state called get_entity_state(entity_id) where entity_id was str | None -- safe today only because the one call site's should_wait guard embeds an entity_id is not None check the type checker can't see through, now made explicit. And the first Protocol draft declared plain (implicitly settable) attributes, which HomeAssistantSmartMCPServer's read-only client/device_tools @Property definitions don't satisfy -- fixed by declaring them as @Property in the Protocol too. Validated with the exact CI mypy command (mypy src/ custom_components/ homeassistant-addon/ scripts/, 255 files) -- zero new errors anywhere else in the codebase. 2. Add test_parameters_copies_are_genuinely_deep_not_shallow: the existing parameters-isolation test only reassigned a top-level scalar key, which a shallow dict(...) copy already protects against. This mutates a nested list value (rgb_color) both externally (caller's dict, after the call) and on one already-returned row, checking the resolution's stored copy and a fresh .operations re-read. Verified load-bearing by temporarily reverting each of the two copy.deepcopy call sites to dict() independently and confirming the test fails both times before restoring the fix. 3. Fix the selector-only-parameter example's placeholder rendering as a quoted string for every field, including timeout_seconds (a float). New _PER_ROW_PARAMETER_EXAMPLE_VALUES gives each field a correctly-typed sample (5 for timeout_seconds, {"brightness_pct": 30} for parameters, etc.) instead of a bare "...". Pinned by test_selector_only_parameter_example_uses_a_correctly_typed_sample. Full unit suite: 10725 passed (up from 10722 by the tests added here); the same 26 pre-existing Windows-platform-only failures as every prior baseline this session remain, unrelated to this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bulk): CodeQL py/ineffectual-statement on the new Protocol's stub bodies The four _ServerLike property stubs used a bare `...` body, which CodeQL's py/ineffectual-statement flags as a discarded-value expression statement. Replaced with one-line docstrings, matching the same stub-body convention already used by CustomRouteServer in browser_landing.py -- mypy still treats a docstring-only body as a valid Protocol stub (no missing-return error), confirmed via the exact CI mypy command across all 255 source files. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent bf47d45 commit 0bb3fa0

20 files changed

Lines changed: 5689 additions & 123 deletions

homeassistant-addon/DOCS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -631,7 +631,7 @@ The add-on provides 88+ MCP tools for controlling Home Assistant:
631631
- `ha_search` — Search for entities (lights, sensors, switches, climate, etc.) by name, domain, or area — AND inside automation/script/scene/helper/dashboard configurations — in one call.
632632

633633
### Service & Device Control
634-
- `ha_bulk_control` — Manage multiple entity actions in one request.
634+
- `ha_bulk_control` — Manage explicit operations or one deterministic structural bulk action.
635635
- `ha_call_event` — Execute a custom event on the Home Assistant event bus.
636636
- `ha_call_service` — Execute Home Assistant services to control entities and trigger automations.
637637
- `ha_get_operation_status` — Get the status of one or more device operations with real-time WebSocket verification.

site/src/data/tools.json

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2988,20 +2988,41 @@
29882988
{
29892989
"name": "ha_bulk_control",
29902990
"title": "Bulk Control",
2991-
"description": "Manage multiple entity actions in one request.\n\nWhen NOT to use: use ``ha_call_service`` for one service call targeting a\ngroup or for service-specific payloads that do not fit device actions.\n\nUse this when one request should apply actions to multiple independent\nentities. Optional item parameters carry brightness, temperature, position,\nor other action data.\n\nCaveats: put every target in ``operations`` and call the tool once. Parallel\nexecution is the default, and invalid items are reported without aborting\nvalid operations in the same batch. A batch in which every item fails\nvalidation dispatches nothing and fails the call.",
2991+
"description": "Manage explicit operations or one deterministic structural bulk action.\n\nWhen NOT to use: use ``ha_call_service`` for service-specific payloads or\nbackend-native group targeting, and ``ha_search`` for fuzzy name discovery.\n\nUse selector mode with exact area or floor IDs when exclusions must be\napplied after recursively expanding generic aggregate membership.\n\nCaveats: selector mode resolves a frozen visible leaf set before dispatch;\nit is not transactional, so Home Assistant may still report per-leaf failures.\nSet ``dry_run`` to preview the resolved set without changing state.",
29922992
"inputSchema": {
29932993
"properties": {
29942994
"operations": {
2995-
"type": "Annotated[list[SkipValidation[BulkControlOperation]], JSON_STRING_COERCION, Field(description=\"All entity operations to execute in this single tool call. Each item requires entity_id and action. Use action='off', not service='turn_off'; do not include domain or service. Example: [{'entity_id': 'light.hall', 'action': 'off'}, {'entity_id': 'light.cave', 'action': 'off'}]\")]"
2995+
"type": "Annotated[list[SkipValidation[BulkControlOperation]], JSON_STRING_COERCION, Field(description=\"Explicit entity operations. Use this or selector, never both. Each item requires exact entity_id and action. Use action='off', not service='turn_off'.\")]"
29962996
},
29972997
"parallel": {
29982998
"type": "bool",
29992999
"default": true
3000+
},
3001+
"selector": {
3002+
"type": "Annotated[SkipValidation[BulkControlSelector] | None, JSON_STRING_COERCION, Field(description='Optional exact structural scope using domain plus area_ids and/or floor_ids, with optional exclude_entity_ids.')]",
3003+
"default": null
3004+
},
3005+
"action": {
3006+
"type": "Annotated[str | None, Field(description='One device action applied to every resolved leaf.')]",
3007+
"default": null
3008+
},
3009+
"parameters": {
3010+
"type": "Annotated[dict[str, Any] | None, JSON_STRING_COERCION, Field(description='Optional action parameters for selector mode.')]",
3011+
"default": null
3012+
},
3013+
"timeout_seconds": {
3014+
"type": "Annotated[float | None, Field(ge=0, le=60, allow_inf_nan=False, strict=True)]",
3015+
"default": null
3016+
},
3017+
"validate_first": {
3018+
"type": "Annotated[bool, Field(strict=True)]",
3019+
"default": true
3020+
},
3021+
"dry_run": {
3022+
"type": "Annotated[bool, Field(strict=True)]",
3023+
"default": false
30003024
}
3001-
},
3002-
"required": [
3003-
"operations"
3004-
]
3025+
}
30053026
},
30063027
"annotations": {
30073028
"openWorldHint": false,

src/ha_mcp/client/rest_client.py

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -409,13 +409,26 @@ async def get_config(self) -> dict[str, Any]:
409409
return await self._request("GET", "/config")
410410

411411
async def get_states(self) -> list[dict[str, Any]]:
412-
"""Get all entity states."""
412+
"""Get all entity states.
413+
414+
Raises ``HomeAssistantConnectionError`` when the response isn't the
415+
JSON array ``/states`` always returns on success -- including
416+
``_request``'s own empty-dict fallback for an unparseable body.
417+
Silently returning ``[]`` here would let a genuine fetch failure
418+
masquerade as "this instance has zero entities", which no real,
419+
running Home Assistant instance produces; callers that need to
420+
verify something against the live entity set (e.g. a bulk-control
421+
safety check) would otherwise treat that failure as "verified,
422+
nothing to worry about" instead of "could not verify".
423+
"""
413424
logger.debug("Fetching all entity states")
414425
result = await self._request("GET", "/states")
415426
if isinstance(result, list):
416427
return result
417-
else:
418-
return []
428+
raise HomeAssistantConnectionError(
429+
f"Home Assistant /states returned an unexpected response shape: "
430+
f"{type(result).__name__}"
431+
)
419432

420433
async def get_entity_state(self, entity_id: str) -> dict[str, Any]:
421434
"""

src/ha_mcp/errors.py

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,16 @@ class ErrorCode(StrEnum):
100100
USER_DENIED = "USER_DENIED"
101101
POLICY_LOAD_FAILED = "POLICY_LOAD_FAILED"
102102

103+
# Distinct from POLICY_LOAD_FAILED above: the policy file itself loaded
104+
# fine, but this call's own arguments were too deeply nested to
105+
# evaluate against it safely (see normalize_stringified_containers'
106+
# RecursionError handling in middleware.py). A different failure with a
107+
# different, caller-shaped remedy ("reduce the nesting depth and
108+
# retry") -- sharing POLICY_LOAD_FAILED would make the two
109+
# indistinguishable to anything grouping on the code (a dashboard, an
110+
# operator grepping logs after "my policy broke").
111+
POLICY_ARGS_TOO_DEEPLY_NESTED = "POLICY_ARGS_TOO_DEEPLY_NESTED"
112+
103113
# Read Only Mode (discussion #1569). A write operation was blocked
104114
# because the server-wide Read Only Mode toggle is on.
105115
READ_ONLY_MODE = "READ_ONLY_MODE"
@@ -210,6 +220,10 @@ class ErrorCode(StrEnum):
210220
"Check documentation for required fields",
211221
"Ensure all required parameters are provided",
212222
],
223+
ErrorCode.VALIDATION_FAILED: [
224+
"Check the parameter values and format against the tool documentation",
225+
"Review the message and details fields for the specific constraint that failed",
226+
],
213227
ErrorCode.VALIDATION_INVALID_JSON: [
214228
"Ensure the parameter is valid JSON",
215229
"Check for syntax errors in JSON",
@@ -310,10 +324,21 @@ def create_connection_error(
310324
details: str | None = None,
311325
timeout: bool = False,
312326
context: dict[str, Any] | None = None,
327+
suggestions: list[str] | None = None,
313328
) -> dict[str, Any]:
314-
"""Create a connection error response."""
329+
"""Create a connection error response.
330+
331+
``suggestions`` overrides ``DEFAULT_SUGGESTIONS[CONNECTION_FAILED/TIMEOUT]``
332+
(network/URL/connectivity checks) for a caller whose failure is
333+
CONNECTION_FAILED-shaped (unavailable, not the caller's fault to fix by
334+
editing input) but not actually a network problem — e.g. malformed local
335+
registry data or an unloadable local config file, where "check your
336+
HOMEASSISTANT_URL" is not an actionable next step.
337+
"""
315338
code = ErrorCode.CONNECTION_TIMEOUT if timeout else ErrorCode.CONNECTION_FAILED
316-
return create_error_response(code, message, details, context=context)
339+
return create_error_response(
340+
code, message, details, suggestions=suggestions, context=context
341+
)
317342

318343

319344
# Authentication-error suggestions for Home Assistant add-on installs. On the

src/ha_mcp/policy/evaluator.py

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from enum import StrEnum
77
from typing import Any
88

9+
from ..tools.util_helpers import loads_if_json_container_str
910
from .model import Policy, Predicate, Rule
1011

1112
logger = logging.getLogger(__name__)
@@ -16,6 +17,82 @@ class Verdict(StrEnum):
1617
REQUIRE_APPROVAL = "require_approval"
1718

1819

20+
def normalize_stringified_containers(value: Any) -> Any:
21+
"""Recursively parse JSON-encoded object/array strings into real containers.
22+
23+
Some MCP client stacks (Claude Desktop stdio among them — see
24+
``tools/util_helpers.py``'s ``JSON_STRING_COERCION``) pass model-emitted
25+
stringified objects through unrepaired, e.g. sending
26+
``{"selector": "{\\"domain\\": \\"light\\"}"}`` instead of a nested
27+
object. Pydantic's ``JSON_STRING_COERCION`` ``BeforeValidator`` repairs
28+
this, but only when the tool's own parameter validation runs — INSIDE
29+
``call_next``, after policy evaluation. ``iter_path_values`` only
30+
descends into ``dict``/``list``, so a still-stringified value makes a
31+
predicate targeting a nested field (``args.selector.domain``,
32+
``args.operations.*.entity_id``) silently yield nothing: no rule
33+
matches, and — for ``ha_bulk_control`` — ``args.get("selector")`` still
34+
sees a truthy string, so the selector fail-safe's own dynamic-call
35+
detection still fires, but nothing inside it can be inspected either.
36+
Applying the same repair here, once, before evaluation and hashing,
37+
closes that gap for both plain predicate rules and the fail-safe, and
38+
makes ``compute_args_hash`` key on the same logical value regardless of
39+
which wire shape the client sent.
40+
41+
Malformed JSON that merely looks like a container is left as the raw
42+
string (not raised): policy evaluation is not the place to surface a
43+
JSON syntax error — the tool's own validation does that, with a
44+
properly attributed parameter name.
45+
46+
Deeply-nested INPUT (real nested dicts/lists in the caller's own
47+
``args`` -- NOT a stringified container decoding into deep nesting;
48+
``loads_if_json_container_str`` already absorbs ``json.loads``'s own
49+
``RecursionError``, so a still-stringified value can never reach this
50+
function's own recursion at all) can exceed the interpreter's
51+
recursion limit and raise ``RecursionError`` here. Unlike the
52+
malformed-JSON case, this is deliberately NOT swallowed: silently
53+
returning the unrepaired value would let this security gate evaluate
54+
(and hash) unnormalized args and, for a rule scoped to a nested
55+
selector-inspectable field, silently ALLOW a call that should have
56+
required approval -- the identical "fail open on a degraded gate"
57+
mistake ``PolicyMiddleware.on_call_tool`` already refuses to make for
58+
a corrupt policy file. The caller (``on_call_tool``) is responsible
59+
for catching this and failing the call closed with a structured,
60+
logged error, matching that same precedent.
61+
"""
62+
return _normalize_stringified_containers(value)
63+
64+
65+
def _normalize_stringified_containers(value: Any) -> Any:
66+
"""Unbounded recursive worker for ``normalize_stringified_containers``.
67+
68+
Split out so the public function's ``RecursionError`` guard wraps a
69+
single top-level call instead of needing a try/except at every
70+
recursive frame.
71+
"""
72+
if isinstance(value, str):
73+
try:
74+
return loads_if_json_container_str(value)
75+
except ValueError:
76+
return value
77+
if isinstance(value, dict):
78+
return {key: _normalize_stringified_containers(v) for key, v in value.items()}
79+
if isinstance(value, list):
80+
return [_normalize_stringified_containers(v) for v in value]
81+
return value
82+
83+
84+
def has_dynamic_selector_targets(name: str, args: dict[str, Any]) -> bool:
85+
"""Return whether identical arguments can resolve to different targets later.
86+
87+
Single source of truth for the ``ha_bulk_control`` selector-mode predicate:
88+
both ``PolicyMiddleware`` (approval-sharing/remembering gates) and
89+
``evaluate()`` below (the operations fail-safe) must agree on exactly
90+
which calls are "dynamic", or a future rename/extension of this check in
91+
one place silently stops applying to the other.
92+
"""
93+
return name == "ha_bulk_control" and args.get("selector") is not None
94+
95+
1996
def iter_path_values(args: dict[str, Any], path: str) -> Iterator[Any]:
2097
"""Yield every value the dotted path resolves to.
2198
@@ -142,7 +219,65 @@ def find_matching_rule(
142219
return None
143220

144221

222+
def _predicate_reaches_operations(path: str) -> bool:
223+
"""Whether ``path`` can walk into ``args.operations`` under ``iter_path_values``.
224+
225+
``operations`` sits directly under ``args``, so only the first two
226+
segments after stripping the implicit ``args`` prefix matter. A literal
227+
``operations`` first segment obviously reaches it. A leading ``*`` reaches
228+
it too — a wildcard segment fans out over EVERY value at that level (see
229+
``iter_path_values``), landing on the `operations` list value exactly as
230+
readily as any other top-level key — but ``operations`` is a *list*, so a
231+
literal segment right after that wildcard (e.g. ``domain`` in
232+
``args.*.domain``) can only ever match a *dict* value at that level (like
233+
``selector``) — ``walk()`` requires ``isinstance(cur, dict)`` for a
234+
literal head, so it silently yields nothing against a list and can never
235+
reach an operation row. Only a SECOND wildcard (``args.*.*...``, as in
236+
``args.*.*.entity_id``) or no further segment at all (bare ``args.*``,
237+
which yields the raw ``operations`` list value itself) can actually reach
238+
into the list. Any other concrete first segment (e.g. ``selector``) can
239+
only ever address selector-inspectable fields and is precisely excluded.
240+
"""
241+
parts = path.split(".")
242+
if parts and parts[0] == "args":
243+
parts = parts[1:]
244+
if not parts:
245+
return False
246+
if parts[0] == "operations":
247+
return True
248+
return parts[0] == "*" and (len(parts) == 1 or parts[1] == "*")
249+
250+
251+
def _rule_needs_resolved_operations(rule: Rule) -> bool:
252+
"""Whether ``rule`` inspects fields only known after selector resolution.
253+
254+
A selector-mode ``ha_bulk_control`` call carries ``args.selector``, not
255+
``args.operations`` — the leaf targets don't exist yet, they're resolved
256+
inside the tool after this middleware runs. A rule predicate that can
257+
reach ``args.operations`` (exact, prefixed, or via a leading wildcard —
258+
see ``_predicate_reaches_operations``) can therefore never get a fair
259+
match attempt against a selector call and needs the fail-safe below. A
260+
rule whose predicates only ever address selector-inspectable fields
261+
(e.g. ``args.selector.domain``) already got a fair, precise match
262+
attempt in ``find_matching_rule`` and must not be broadened into an
263+
unconditional gate.
264+
"""
265+
return any(_predicate_reaches_operations(p.path) for p in rule.when)
266+
267+
145268
def evaluate(tool_name: str, args: dict[str, Any], policy: Policy) -> Verdict:
269+
"""Decide whether one tool call requires approval under ``policy``.
270+
271+
A normal rule match (``find_matching_rule``) decides most calls. Two
272+
fail-safes broaden approval beyond an exact predicate match, each only
273+
when the operator has SOME rule that could plausibly apply: an
274+
unmatched ``ha_call_service`` ``ws_command`` call (no ``domain``/
275+
``service`` args for a domain/service-keyed rule to match), and an
276+
``ha_bulk_control`` selector call whose matching rule needs fields
277+
(``args.operations.*``) that don't exist yet at gate time, resolved
278+
only later inside the tool. See the fail-safe blocks below for the
279+
full reasoning behind each.
280+
"""
146281
if find_matching_rule(tool_name, args, policy) is not None:
147282
return Verdict.REQUIRE_APPROVAL
148283
# ha_call_service exposes a raw WebSocket escape hatch (``ws_command``) that
@@ -163,4 +298,17 @@ def evaluate(tool_name: str, args: dict[str, Any], policy: Policy) -> Verdict:
163298
and any(rule.tool_name in ("ha_call_service", "*") for rule in policy.rules)
164299
):
165300
return Verdict.REQUIRE_APPROVAL
301+
# Structural selectors are resolved inside the tool, after this middleware.
302+
# A pre-existing rule that inspects args.operations.* therefore cannot inspect
303+
# the eventual leaf targets. Fail safe only for rules that actually depend on
304+
# that unresolved data — a rule fully expressed over selector-inspectable
305+
# fields (e.g. args.selector.domain) already had its precise shot at matching
306+
# above, and broadening it here would defeat a deliberately conditional rule
307+
# (a rule scoped to selector.domain == "lock" must not gate a "light" call).
308+
if has_dynamic_selector_targets(tool_name, args) and any(
309+
rule.tool_name in ("ha_bulk_control", "*")
310+
and _rule_needs_resolved_operations(rule)
311+
for rule in policy.rules
312+
):
313+
return Verdict.REQUIRE_APPROVAL
166314
return Verdict.ALLOW

0 commit comments

Comments
 (0)