Skip to content

Commit 89c082b

Browse files
julienldclaude
julienld
and
claude
authored
fix: preserve 'conditions' (plural) in choose/if blocks (#388)
* fix: preserve 'conditions' (plural) in choose/if blocks BREAKING BUG FIX: The _normalize_automation_config function was incorrectly converting 'conditions' (plural) to 'condition' (singular) inside choose and if action blocks, causing API validation failures: "extra keys not allowed @ data['actions'][0]['choose'][0]['condition']" Root Cause: The normalization was applied recursively at ALL levels without context awareness. Home Assistant requires 'conditions' (plural) in choose/if blocks, but the function was blindly converting it. Fix: - Add context tracking (parent_key, in_choose_or_if parameters) - Only normalize 'conditions' → 'condition' at root automation level - Preserve 'conditions' (plural) inside choose/if action blocks - Continue normalizing other plural keys (triggers/actions/sequences) Testing: - 8 new unit tests covering all normalization scenarios - 1 new E2E test with real Home Assistant API validation - All existing tests pass Fixes issues reported in: - /home/julien/github/perso/ha-mcp/debug/choose_block_validation_bug.md - /home/julien/github/perso/ha-mcp/debug/tool_improvement_suggestions.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * test: fix E2E test parameter names and assertions - Use 'identifier' parameter instead of 'automation_id' - Extract config from nested response structure - Test successfully validates choose block normalization All tests passing: - 8/8 unit tests ✅ - 1/1 E2E test ✅ * fix: add polling wait for script traces in E2E test Fixes ARM platform test failure where script traces weren't immediately available after execution. Added wait_for_condition polling with 10s timeout to ensure traces are recorded before assertions. Fixes test_script_traces sporadic failures on ubuntu-24.04-arm. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * fix: address code review feedback - Fix critical bug where in_choose_or_if flag was incorrectly passed recursively Only direct children of choose/if options should preserve 'conditions' plural - Add missing unit test for conditions normalization inside sequence blocks - Simplify E2E test to focus on normalization verification Addresses all Gemini Code Assist review comments on PR #388. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent e08b8be commit 89c082b

4 files changed

Lines changed: 433 additions & 9 deletions

File tree

‎src/ha_mcp/tools/tools_config_automations.py‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,25 +21,38 @@
2121
logger = logging.getLogger(__name__)
2222

2323

24-
def _normalize_automation_config(config: Any) -> Any:
24+
def _normalize_automation_config(
25+
config: Any, parent_key: str | None = None, in_choose_or_if: bool = False
26+
) -> Any:
2527
"""
2628
Recursively normalize automation config field names to HA API format.
2729
2830
Home Assistant accepts both singular ('trigger', 'action', 'condition')
2931
and plural ('triggers', 'actions', 'conditions') field names in YAML,
30-
but the API expects singular forms. This function normalizes plural
31-
to singular for consistency, recursively processing nested structures
32-
like 'choose', 'repeat', 'if/then/else'.
32+
but the API expects singular forms at the root level.
33+
34+
IMPORTANT: Inside 'choose' and 'if' action blocks, the 'conditions' key
35+
(plural) is required by the HA schema and should NOT be normalized to
36+
'condition' (singular).
3337
3438
Args:
3539
config: Automation configuration (dict, list, or primitive)
40+
parent_key: The parent dictionary key (for context tracking)
41+
in_choose_or_if: Whether we're inside a choose/if option that requires
42+
'conditions' (plural) to remain unchanged
3643
3744
Returns:
38-
Normalized configuration with singular field names
45+
Normalized configuration with singular field names at root level,
46+
but preserving 'conditions' (plural) inside choose/if blocks
3947
"""
4048
# Handle lists - recursively process each item
4149
if isinstance(config, list):
42-
return [_normalize_automation_config(item) for item in config]
50+
# If parent is 'choose' or 'if', items are options that need 'conditions' preserved
51+
is_option_list = parent_key in ("choose", "if")
52+
return [
53+
_normalize_automation_config(item, parent_key, is_option_list)
54+
for item in config
55+
]
4356

4457
# Handle primitives (strings, numbers, etc.)
4558
if not isinstance(config, dict):
@@ -49,14 +62,18 @@ def _normalize_automation_config(config: Any) -> Any:
4962
normalized = config.copy()
5063

5164
# Map plural field names to singular (HA API format)
65+
# EXCEPT 'conditions' when inside choose/if blocks
5266
field_mappings = {
5367
"triggers": "trigger",
5468
"actions": "action",
55-
"conditions": "condition",
5669
# Note: 'sequence' is already singular, but some users might use 'sequences'
5770
"sequences": "sequence",
5871
}
5972

73+
# Only add 'conditions' mapping if NOT inside a choose/if option
74+
if not in_choose_or_if:
75+
field_mappings["conditions"] = "condition"
76+
6077
# Apply field mapping to current level
6178
for plural, singular in field_mappings.items():
6279
if plural in normalized and singular not in normalized:
@@ -66,8 +83,9 @@ def _normalize_automation_config(config: Any) -> Any:
6683
del normalized[plural]
6784

6885
# Recursively process all values in the dictionary
86+
# Note: Don't pass in_choose_or_if flag down - it only applies to direct children
6987
for key, value in normalized.items():
70-
normalized[key] = _normalize_automation_config(value)
88+
normalized[key] = _normalize_automation_config(value, key)
7189

7290
return normalized
7391

‎tests/src/e2e/workflows/automation/test_lifecycle.py‎

Lines changed: 169 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
wait_for_automation,
2020
)
2121
from ...utilities.wait_helpers import (
22-
wait_for_condition,
2322
wait_for_entity_state,
2423
wait_for_logbook_entry,
2524
)
@@ -834,3 +833,172 @@ async def test_automation_search_and_discovery(mcp_client):
834833
logger.info(f"🔍 Pattern '{pattern}' search: {len(results)} results")
835834

836835
logger.info("✅ Automation search and discovery tests completed")
836+
837+
838+
839+
@pytest.mark.automation
840+
async def test_automation_with_choose_block(mcp_client):
841+
"""
842+
Test automation with choose blocks to verify conditions (plural) is preserved.
843+
844+
This test ensures that the normalization bug is fixed where 'conditions'
845+
was incorrectly being converted to 'condition' inside choose blocks,
846+
causing API validation failures.
847+
"""
848+
logger.info("🧪 Testing automation with choose block...")
849+
850+
# Find a test light entity
851+
search_result = await mcp_client.call_tool(
852+
"ha_search_entities",
853+
{"query": "light", "domain_filter": "light", "limit": 5},
854+
)
855+
search_data = parse_mcp_result(search_result)
856+
857+
# Handle nested data structure
858+
if "data" in search_data:
859+
entities = search_data.get("data", {}).get("results", [])
860+
else:
861+
entities = search_data.get("results", [])
862+
863+
assert len(entities) > 0, "No light entities found for testing"
864+
light_entity = entities[0]["entity_id"]
865+
logger.info(f"🔦 Using test light: {light_entity}")
866+
867+
automation_id = "test_choose_block_normalization"
868+
869+
# Create automation with choose block that has conditions (plural)
870+
config = {
871+
"alias": "Test Choose Block Normalization",
872+
"description": "Test that choose block conditions (plural) are preserved",
873+
"triggers": [ # Using plural to test normalization
874+
{
875+
"platform": "state",
876+
"entity_id": light_entity,
877+
"to": "on",
878+
"id": "light_on",
879+
},
880+
{
881+
"platform": "state",
882+
"entity_id": light_entity,
883+
"to": "off",
884+
"id": "light_off",
885+
},
886+
],
887+
"actions": [ # Using plural to test normalization
888+
{
889+
"choose": [
890+
{
891+
"conditions": [ # MUST remain plural in choose blocks
892+
{
893+
"condition": "trigger",
894+
"id": "light_on",
895+
}
896+
],
897+
"sequences": [ # Test sequence normalization too
898+
{
899+
"service": "persistent_notification.create",
900+
"data": {
901+
"title": "Choose Test",
902+
"message": "Light turned on",
903+
},
904+
}
905+
],
906+
},
907+
{
908+
"conditions": [ # MUST remain plural
909+
{
910+
"condition": "trigger",
911+
"id": "light_off",
912+
}
913+
],
914+
"sequence": [ # Test singular form too
915+
{
916+
"service": "persistent_notification.create",
917+
"data": {
918+
"title": "Choose Test",
919+
"message": "Light turned off",
920+
},
921+
}
922+
],
923+
},
924+
],
925+
"default": [
926+
{
927+
"service": "persistent_notification.create",
928+
"data": {
929+
"title": "Choose Test",
930+
"message": "Default action",
931+
},
932+
}
933+
],
934+
}
935+
],
936+
}
937+
938+
# Create the automation - THIS IS THE KEY TEST
939+
# If normalization is broken, this will fail with:
940+
# "extra keys not allowed @ data['actions'][0]['choose'][0]['condition']"
941+
logger.info("📝 Creating automation with choose block...")
942+
create_result = await mcp_client.call_tool(
943+
"ha_config_set_automation",
944+
{
945+
"identifier": automation_id,
946+
"config": config,
947+
},
948+
)
949+
950+
assert_mcp_success(create_result)
951+
logger.info("✅ Automation with choose block created successfully")
952+
953+
# Wait for automation to be registered
954+
await wait_for_automation(mcp_client, automation_id)
955+
956+
# Retrieve the automation to verify structure
957+
get_result = await mcp_client.call_tool(
958+
"ha_config_get_automation",
959+
{"identifier": automation_id},
960+
)
961+
962+
automation_data = parse_mcp_result(get_result)
963+
logger.info("📥 Retrieved automation configuration")
964+
965+
# Extract config from response
966+
config_data = automation_data.get("config", automation_data)
967+
968+
# Verify the automation has the correct structure
969+
assert "trigger" in config_data or "triggers" in config_data, (
970+
"Automation should have triggers"
971+
)
972+
973+
actions = config_data.get("action", config_data.get("actions", []))
974+
assert len(actions) > 0, "Automation should have actions"
975+
976+
choose_action = actions[0]
977+
assert "choose" in choose_action, "First action should be a choose block"
978+
assert len(choose_action["choose"]) == 2, "Choose should have 2 options"
979+
980+
# Verify that conditions are preserved in choose options
981+
for i, option in enumerate(choose_action["choose"]):
982+
# The key could be 'conditions' or 'condition' depending on HA version
983+
# But our normalization should have sent 'conditions' to the API
984+
has_conditions = "conditions" in option or "condition" in option
985+
assert has_conditions, (
986+
f"Choose option {i} should have conditions defined"
987+
)
988+
logger.info(f"✅ Choose option {i} has condition key: {list(option.keys())}")
989+
990+
# The fact that we successfully created and retrieved the automation
991+
# with choose blocks proves the normalization fix works.
992+
# Execution testing would require more complex setup (triggering actual
993+
# entity state changes) which is beyond the scope of this normalization test.
994+
logger.info("✅ Choose block normalization verified - automation API accepted the config")
995+
996+
# Clean up
997+
logger.info("🧹 Cleaning up test automation...")
998+
delete_result = await mcp_client.call_tool(
999+
"ha_config_remove_automation",
1000+
{"identifier": automation_id},
1001+
)
1002+
assert_mcp_success(delete_result)
1003+
1004+
logger.info("✅ Choose block normalization test completed successfully")

‎tests/src/e2e/workflows/automation/test_traces.py‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
Verifies that ha_get_automation_traces returns non-empty traces after automation runs.
66
"""
77

8+
import asyncio
89
import logging
910

1011
import pytest
@@ -13,6 +14,7 @@
1314
assert_mcp_success,
1415
parse_mcp_result,
1516
)
17+
from ...utilities.wait_helpers import wait_for_condition
1618

1719
logger = logging.getLogger(__name__)
1820

@@ -263,6 +265,21 @@ async def test_script_traces(self, mcp_client, cleanup_tracker, test_data_factor
263265
logger.info("Script executed")
264266

265267
# Wait for trace to be recorded
268+
async def check_traces():
269+
result = await mcp_client.call_tool(
270+
"ha_get_automation_traces",
271+
{"automation_id": script_entity_id},
272+
)
273+
data = parse_mcp_result(result)
274+
return data.get("trace_count", 0) > 0
275+
276+
logger.info("Waiting for script trace to be recorded...")
277+
await wait_for_condition(
278+
check_traces,
279+
timeout=10,
280+
poll_interval=0.5,
281+
condition_name="script trace to be recorded"
282+
)
266283

267284
# 4. Get traces for the script
268285
traces_result = await mcp_client.call_tool(

0 commit comments

Comments
 (0)