Skip to content

Commit 0d7151e

Browse files
fix: add post-operation verification to group config tools (#853)
* fix: add post-operation verification to group config tools Apply the existing wait_for_entity_registered/removed pattern (already used by automations, scripts, and helpers) to group config tools: - ha_config_set_group: verify group.{object_id} entity is queryable after group.set service call - ha_config_remove_group: verify entity is removed after group.remove service call Groups use fire-and-forget service calls (group.set/group.remove) that acknowledge the command before the entity state updates, which can cause false success responses. This is the same pattern that was already fixed for automations (#708) and scripts. Other tools flagged in #709 (zones, labels, areas) use synchronous WebSocket registry operations that return the actual result data, so they don't have the same false-success risk. Partial fix for #709 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add post-operation verification to zone config tools - ha_set_zone (create): re-query zone/list to verify new zone appears in registry - ha_remove_zone: re-query zone/list to verify zone is gone Zones use WebSocket registry operations that are synchronous, but the zone entity (zone.xxx) registration can lag. Re-querying the registry confirms the operation persisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert: remove redundant zone registry re-query verification Gemini correctly identified that re-querying zone/list after a synchronous WebSocket zone/create is redundant — the success response already confirms the registry update. The real state machine lag (zone.xxx entity registration) would need wait_for_entity_registered, but zone entity_ids can't be reliably predicted from the name. Keep groups verification only — group.set is a fire-and-forget service call where entity_id IS predictable (group.{object_id}). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add post-operation verification to integration config tools - ha_set_integration_enabled: re-query config entry to verify disabled_by field matches the requested state (skipped when require_restart=True since change is deferred) - ha_delete_config_entry: re-query config entry to verify it's actually gone (404 = expected success, skipped when require_restart=True) These operations use synchronous REST/WebSocket calls that are more reliable than service calls, but the issue (#709) flagged them as high-risk since they report success without confirming the operation's side effects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert: remove unnecessary integration verification Integration tools use synchronous REST/WebSocket calls that either succeed or throw — no false-success scenario exists. The issue's "high risk" classification was wrong for these tools. Only fire-and-forget service calls (like group.set) can produce false successes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add regression tests for group verification - test_created_group_is_immediately_queryable: after ha_config_set_group returns success, verify the entity is actually queryable via ha_get_state - test_removed_group_is_immediately_gone: after ha_config_remove_group returns success, verify the entity is no longer queryable These tests would fail if the wait_for_entity_registered/removed verification were reverted, confirming the fix prevents false success responses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: handle nested data key in group verification test ha_get_state response wraps entity data under a "data" key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c3d726a commit 0d7151e

2 files changed

Lines changed: 148 additions & 0 deletions

File tree

src/ha_mcp/tools/tools_groups.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@
1313

1414
from ..errors import ErrorCode, create_error_response
1515
from .helpers import exception_to_structured_error, log_tool_usage, raise_tool_error
16+
from .util_helpers import (
17+
coerce_bool_param,
18+
wait_for_entity_registered,
19+
wait_for_entity_removed,
20+
)
1621

1722
logger = logging.getLogger(__name__)
1823

@@ -133,6 +138,13 @@ async def ha_config_set_group(
133138
default=None,
134139
),
135140
] = None,
141+
wait: Annotated[
142+
bool | str,
143+
Field(
144+
description="Wait for group to be queryable before returning. Default: True. Set to False for bulk operations.",
145+
default=True,
146+
),
147+
] = True,
136148
) -> dict[str, Any]:
137149
"""
138150
Create or update a Home Assistant entity group.
@@ -225,12 +237,24 @@ async def ha_config_set_group(
225237
# Determine if this was a create or update based on fields provided
226238
is_create = entities is not None and name is None and add_entities is None and remove_entities is None
227239

240+
# Verify entity is queryable after creation/update
241+
wait_bool = coerce_bool_param(wait, "wait", default=True)
242+
result: dict[str, Any] = {}
243+
if wait_bool:
244+
try:
245+
registered = await wait_for_entity_registered(client, entity_id)
246+
if not registered:
247+
result["warning"] = f"Group created but {entity_id} not yet queryable. It may take a moment to become available."
248+
except Exception as e:
249+
result["warning"] = f"Group created but verification failed: {e}"
250+
228251
return {
229252
"success": True,
230253
"entity_id": entity_id,
231254
"object_id": object_id,
232255
"updated_fields": updated_fields,
233256
"message": f"Successfully {'created' if is_create else 'updated'} group: {entity_id}",
257+
**result,
234258
}
235259

236260
except ToolError:
@@ -253,6 +277,13 @@ async def ha_config_remove_group(
253277
description="Group identifier without 'group.' prefix (e.g., 'living_room_lights')"
254278
),
255279
],
280+
wait: Annotated[
281+
bool | str,
282+
Field(
283+
description="Wait for group to be fully removed before returning. Default: True.",
284+
default=True,
285+
),
286+
] = True,
256287
) -> dict[str, Any]:
257288
"""
258289
Remove a Home Assistant entity group.
@@ -285,11 +316,23 @@ async def ha_config_remove_group(
285316

286317
entity_id = f"group.{object_id}"
287318

319+
# Verify entity is removed
320+
wait_bool = coerce_bool_param(wait, "wait", default=True)
321+
result: dict[str, Any] = {}
322+
if wait_bool:
323+
try:
324+
removed = await wait_for_entity_removed(client, entity_id)
325+
if not removed:
326+
result["warning"] = f"Deletion confirmed by API but {entity_id} may still appear briefly."
327+
except Exception as e:
328+
result["warning"] = f"Deletion confirmed but removal verification failed: {e}"
329+
288330
return {
289331
"success": True,
290332
"entity_id": entity_id,
291333
"object_id": object_id,
292334
"message": f"Successfully removed group: {entity_id}",
335+
**result,
293336
}
294337

295338
except ToolError:
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
"""
2+
Regression tests for group config tool post-operation verification.
3+
4+
Verifies that group tools confirm entity state after service calls,
5+
preventing the false-success anti-pattern where the tool returns
6+
success before the entity is actually queryable.
7+
"""
8+
9+
import logging
10+
11+
import pytest
12+
13+
from ...utilities.assertions import assert_mcp_success, safe_call_tool
14+
15+
logger = logging.getLogger(__name__)
16+
17+
18+
@pytest.mark.group
19+
class TestGroupVerification:
20+
"""Verify that group operations confirm entity state."""
21+
22+
async def test_created_group_is_immediately_queryable(
23+
self, mcp_client, cleanup_tracker
24+
):
25+
"""After ha_config_set_group succeeds, the entity must be queryable.
26+
27+
Regression test: before verification was added, the tool returned
28+
success with a predicted entity_id that might not exist yet.
29+
"""
30+
object_id = "test_e2e_verify_create"
31+
32+
result = await mcp_client.call_tool(
33+
"ha_config_set_group",
34+
{
35+
"object_id": object_id,
36+
"name": "Verification Test Group",
37+
"entities": ["light.bed_light"],
38+
},
39+
)
40+
41+
data = assert_mcp_success(result, "Create group")
42+
entity_id = data.get("entity_id")
43+
assert entity_id == f"group.{object_id}"
44+
cleanup_tracker.track("group", object_id)
45+
46+
# The entity must be queryable immediately after the tool returns
47+
state_result = await mcp_client.call_tool(
48+
"ha_get_state", {"entity_id": entity_id}
49+
)
50+
state_data = assert_mcp_success(state_result, "Get group state after create")
51+
# Response may nest entity data under "data" key
52+
inner = state_data.get("data", state_data)
53+
assert inner.get("entity_id") == entity_id, (
54+
f"Created group not queryable immediately after tool returned success: {state_data}"
55+
)
56+
logger.info(f"Group {entity_id} confirmed queryable after create")
57+
58+
# Cleanup
59+
await mcp_client.call_tool(
60+
"ha_config_remove_group", {"object_id": object_id}
61+
)
62+
63+
async def test_removed_group_is_immediately_gone(
64+
self, mcp_client, cleanup_tracker
65+
):
66+
"""After ha_config_remove_group succeeds, the entity must be gone.
67+
68+
Regression test: before verification was added, the tool returned
69+
success but the entity could still be queryable briefly.
70+
"""
71+
object_id = "test_e2e_verify_remove"
72+
73+
# Create a group first
74+
result = await mcp_client.call_tool(
75+
"ha_config_set_group",
76+
{
77+
"object_id": object_id,
78+
"name": "Removal Verification Group",
79+
"entities": ["light.bed_light"],
80+
},
81+
)
82+
assert_mcp_success(result, "Create group for removal test")
83+
cleanup_tracker.track("group", object_id)
84+
85+
# Remove it
86+
remove_result = await mcp_client.call_tool(
87+
"ha_config_remove_group", {"object_id": object_id}
88+
)
89+
assert_mcp_success(remove_result, "Remove group")
90+
91+
# The entity must NOT be queryable after the tool returns
92+
entity_id = f"group.{object_id}"
93+
state_data = await safe_call_tool(
94+
mcp_client, "ha_get_state", {"entity_id": entity_id}
95+
)
96+
# Should either fail or return not_found
97+
is_gone = (
98+
not state_data.get("success")
99+
or state_data.get("state") == "unavailable"
100+
or "not found" in str(state_data).lower()
101+
)
102+
assert is_gone, (
103+
f"Removed group still queryable after tool returned success: {state_data}"
104+
)
105+
logger.info(f"Group {entity_id} confirmed gone after remove")

0 commit comments

Comments
 (0)