Skip to content

Commit b66e8fc

Browse files
fix(search): floor-area and bulk-control hardening follow-ups (#2243)
* fix: name the cost of a floor-registry outage in area resolution A dead floor registry suppresses all fuzzy area matching, but the only line the caller saw was the generic fetch warning. Return a resolution warning that names the consequence and the recovery, and derive the availability flag from the parse outcome so the two predicates cannot desync. * fix: route ha_search partial_reason through the shared merger _finalize_partial_state concatenated with its own separator and no de-duplication while the module's other three writers used _merge_partial_reason. Also fall back to the exception type name when a branch exception's message is empty, so a timeout no longer produces partial_reason 'entities: '. * refactor: fold the bulk-row skip blocks into one helper Seven copies of log/build/append collapse into _skip_bulk_operation, and the dead err_response['index'] writes go with them — create_error_response already lifts every context key to the top level. The allowed-key literal now reads its keys back from BulkControlOperation so the runtime validator cannot drift from the advertised schema. * fix: reject boolean bulk timeouts at both validation layers Pydantic's lax mode coerces true to 1.0 and float(True) is 1.0, so timeout_seconds: true was silently accepted as a one-second wait on both the transport schema and the runtime batch validator. The field now carries strict=True (matching validate_first) and the runtime helper rejects bools before float() sees them; its return collapses to float | None so the caller no longer re-invalidates None. A swallowed ValidationError in ha_bulk_control now logs the schema's reason, which is the only place that detail survives. * fix: fail a bulk batch whose every operation is invalid An empty operations list already raised, but a batch where every row failed validation returned the success-shaped response with successful_commands: 0. The batch-item carve-out from AGENTS.md's raise-on-failure rule exists to protect successful siblings, and there are none — so raise with the skipped rows in the payload, still ahead of the component probe so nothing dispatches. Mixed batches keep failing soft. * fix: name where a bulk parameters JSON string failed to parse The JSONDecodeError was caught unbound, so the caller was told only that its parameters were invalid JSON. Thread the decoder's reason and offset into the message, and lead every bulk validation-failure suggestion list with the row-shape guidance models get wrong most often. * docs: correct the model-facing bulk descriptions and stale comments validate_first said it prevents dispatch, but the component batch path detects the missing entity from the captured pre-state afterwards. The parameters description never said the per-domain key list is a silent allowlist, and _DOMAIN_PARAMS carried no warning that a new parameter disappears at dispatch until it is added there. Also restores the exact-first rationale on _match_area_ids and reflects the shortened final poll in get_device_operation_status. * test: cover the bulk and floor-expansion branches nothing reached Pins entity aggregation through floor expansion (a second area on the floor with its own entity), a bare floor_id as the area filter, the non-string entity_id branch, and a parameters string that parses to a list rather than an object. * chore: satisfy CodeQL implicit-concat check in bulk suggestions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD * fix: reject string bulk timeouts at runtime to match the strict schema Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD * fix: skip oversized bulk timeouts instead of aborting the batch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD * fix: reject null bulk parameters and classify non-dict rows as invalid Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD * fix: skip pathologically nested bulk parameters instead of aborting the batch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD * fix: stringify non-string bulk operation keys before sorting Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD * fix: redact logged bulk inputs and treat a missing registry result as an outage Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xih4pUWhifP1zhQe3iaKVD --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 0d15c19 commit b66e8fc

10 files changed

Lines changed: 574 additions & 174 deletions

src/ha_mcp/tools/device_control.py

Lines changed: 166 additions & 109 deletions
Large diffs are not rendered by default.

src/ha_mcp/tools/smart_search/_base.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,14 @@ def _extract_registry_list(
4040
logger.debug(f"Could not fetch {label}: {result}")
4141
cause = str(result) or type(result).__name__
4242
elif isinstance(result, dict) and result.get("success"):
43-
payload = result.get("result", [])
43+
# No default for a missing key: a legitimate empty registry is
44+
# "result": [], so an absent result is a malformed envelope and
45+
# must degrade loudly, not read as available-and-empty.
46+
payload = result.get("result")
4447
if isinstance(payload, list):
4548
registry = payload
49+
elif payload is None and "result" not in result:
50+
cause = "malformed result payload: result key missing"
4651
else:
4752
cause = (
4853
"malformed result payload: expected list, "

src/ha_mcp/tools/smart_search/_entities.py

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -473,13 +473,15 @@ async def get_entities_by_area(
473473
area_registry = self._parse_area_registry(results[1], registry_warnings)
474474
entity_reg_map = self._parse_entity_reg_map(results[2], registry_warnings)
475475
device_area_map = self._parse_device_area_map(results[3], registry_warnings)
476-
floor_result = results[4]
477-
floor_registry_available = (
478-
isinstance(floor_result, dict)
479-
and floor_result.get("success") is True
480-
and isinstance(floor_result.get("result"), list)
481-
)
482-
floor_registry = self._parse_floor_registry(floor_result, registry_warnings)
476+
# Availability is read back OUT of the parse rather than re-derived
477+
# from the raw payload: the parser already decides what counts as
478+
# usable floor data, and a second predicate here drifted from it
479+
# once. An empty warnings list means the parser accepted the
480+
# payload; anything else disables the fuzzy floor/area fallback.
481+
floor_warnings: list[str] = []
482+
floor_registry = self._parse_floor_registry(results[4], floor_warnings)
483+
floor_registry_available = not floor_warnings
484+
registry_warnings.extend(floor_warnings)
483485
degraded_warnings = registry_warnings + visibility_warnings
484486

485487
matched_area_ids, resolution_warnings = self._resolve_area_query(
@@ -657,8 +659,15 @@ def _resolve_area_query(
657659

658660
if not floor_registry_available:
659661
# Without the floor registry, fuzzy area matching could silently
660-
# recreate the original floor->partial-area misresolution.
661-
return set(), []
662+
# recreate the original floor->partial-area misresolution. The
663+
# fetch warning names the outage; this one names what the outage
664+
# cost the caller, so "no match" is not read as "no such area".
665+
return set(), [
666+
f"Floor data is unavailable, so '{area_query}' was matched only "
667+
"against exact area IDs, names, and aliases; close-spelling "
668+
"matching was skipped. Retry, or pass an exact area_id from "
669+
"ha_list_floors_areas."
670+
]
662671

663672
fuzzy_floor_ids, floor_score = cls._match_fuzzy_registry_ids(
664673
floor_registry, "floor_id", query_lower
@@ -768,7 +777,10 @@ def _match_area_ids(
768777
"""Resolve the query to area IDs: exact id/name/alias, then fuzzy.
769778
770779
Exact matches delegate to the shared registry matcher so area and floor
771-
precedence use identical case-insensitive semantics.
780+
precedence use identical case-insensitive semantics. Exact must win
781+
outright: a query like "bedroom_kids" partial-matches its parent
782+
"bedroom" at ratio 100, so falling through to the fuzzy pass would
783+
aggregate every sibling area's entities under an exact hit.
772784
"""
773785
exact_area_ids = cls._match_exact_registry_ids(
774786
area_registry, "area_id", area_query_lower

src/ha_mcp/tools/tools_search.py

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -321,12 +321,11 @@ def _finalize_partial_state(
321321
if partial_local:
322322
response["partial"] = True
323323
response["errors"].extend(errors_local)
324-
local_reason = "; ".join(
325-
f"{error['surface']}: {error['error']}" for error in errors_local
326-
)
327-
existing_reason = response.get("partial_reason")
328-
response["partial_reason"] = (
329-
f"{existing_reason}; {local_reason}" if existing_reason else local_reason
324+
_merge_partial_reason(
325+
response,
326+
"; ".join(
327+
f"{error['surface']}: {error['error']}" for error in errors_local
328+
),
330329
)
331330

332331

@@ -2217,7 +2216,11 @@ async def _legacy_ha_search(
22172216
raise outcome
22182217
if isinstance(outcome, Exception):
22192218
partial = True
2220-
errors.append({"surface": label, "error": str(outcome)})
2219+
# ``str(asyncio.TimeoutError())`` is "" — fall back to the type
2220+
# name so partial_reason never reads "entities: ".
2221+
errors.append(
2222+
{"surface": label, "error": str(outcome) or type(outcome).__name__}
2223+
)
22212224
logger.warning("ha_search %s branch failed: %r", label, outcome)
22222225
continue
22232226
_apply_search_outcome(response, label, outcome)

src/ha_mcp/tools/tools_service.py

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,10 @@ class BulkControlOperation(TypedDict):
8383
Field(
8484
description=(
8585
"Optional action parameters, e.g. {'brightness_pct': 30} "
86-
"when action='on'."
86+
"when action='on'. Each domain has a fixed allowlist of "
87+
"supported keys; keys outside it are ignored rather than "
88+
"rejected. Use ha_call_service for parameters this tool "
89+
"does not carry."
8790
)
8891
),
8992
]
@@ -94,6 +97,10 @@ class BulkControlOperation(TypedDict):
9497
Field(
9598
ge=0,
9699
allow_inf_nan=False,
100+
# ``strict`` for the same reason validate_first carries it:
101+
# lax mode coerces ``true`` to 1.0, so a bool would land
102+
# downstream as a silent one-second timeout.
103+
strict=True,
97104
description=(
98105
"Optional confirmation timeout. On the component path, all "
99106
"operations share the maximum requested wait (default 10s, "
@@ -108,8 +115,10 @@ class BulkControlOperation(TypedDict):
108115
Field(
109116
strict=True,
110117
description=(
111-
"Validate that the entity exists before dispatch; default true. "
112-
"The action is always validated."
118+
"Report an ENTITY_NOT_FOUND failure when the target entity "
119+
"does not exist; default true. On the component batch path "
120+
"this is detected from the captured pre-state rather than by "
121+
"preventing dispatch. The action is always validated."
113122
),
114123
),
115124
]
@@ -1531,7 +1540,8 @@ async def ha_bulk_control(
15311540
15321541
Caveats: put every target in ``operations`` and call the tool once. Parallel
15331542
execution is the default, and invalid items are reported without aborting
1534-
valid operations in the same batch.
1543+
valid operations in the same batch. A batch in which every item fails
1544+
validation dispatches nothing and fails the call.
15351545
"""
15361546
parallel_bool = parallel
15371547

@@ -1559,14 +1569,24 @@ async def ha_bulk_control(
15591569
)
15601570

15611571
operations_list: list[Any] = []
1562-
for operation in parsed_operations:
1572+
for index, operation in enumerate(parsed_operations):
15631573
try:
15641574
operations_list.append(
15651575
_BULK_CONTROL_OPERATION_ADAPTER.validate_python(operation)
15661576
)
1567-
except ValidationError:
1577+
except ValidationError as exc:
15681578
# Preserve malformed rows for the runtime batch validator, which
15691579
# reports them in skipped_details instead of rejecting the call.
1580+
# The schema's reason is richer than the runtime validator's and
1581+
# is the only place it survives, so log it here.
1582+
# include_input=False: a malformed row can carry sensitive
1583+
# values (lock or alarm codes in a mistyped field), and
1584+
# str(exc) would write them to persistent server logs.
1585+
logger.warning(
1586+
"ha_bulk_control operation %d failed schema validation: %s",
1587+
index,
1588+
exc.errors(include_url=False, include_input=False),
1589+
)
15701590
operations_list.append(operation)
15711591

15721592
result = await self._device_tools.bulk_device_control(

tests/src/unit/test_area_filter_search.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,16 @@ def _basement_client(*, area_aliases: list[str] | None = None):
360360
],
361361
)
362362

363+
@staticmethod
364+
def _entity_ids(result: dict) -> set[str]:
365+
"""Collect every entity id across a domain-grouped area result."""
366+
return {
367+
record["entity_id"]
368+
for area in result["areas"].values()
369+
for records in area["entities"].values()
370+
for record in records
371+
}
372+
363373
@pytest.mark.asyncio
364374
async def test_floor_name_expands_to_all_areas_on_floor(self):
365375
"""An exact floor name expands instead of fuzzy-matching one area."""
@@ -370,14 +380,46 @@ async def test_floor_name_expands_to_all_areas_on_floor(self):
370380
{"area_id": "garage", "name": "Garage", "floor_id": "ground"},
371381
]
372382
)
383+
# A second area on the floor needs its own entity, or the expansion
384+
# could aggregate zero entities from it and still look correct.
385+
client.entities.append(
386+
{
387+
"entity_id": "light.cave_lamp",
388+
"attributes": {"friendly_name": "Cave Lamp"},
389+
"state": "off",
390+
}
391+
)
392+
client.entity_registry.append(
393+
{"entity_id": "light.cave_lamp", "area_id": "cave", "device_id": None}
394+
)
373395
tools = SmartSearchTools(client=client, fuzzy_threshold=60)
374396

375397
result = await tools.get_entities_by_area("sous-sol")
376398

377399
assert result["total_areas_found"] == 2
378400
assert set(result["areas"]) == {"couloir_sous_sol", "cave"}
401+
assert self._entity_ids(result) == {"light.basement_hall", "light.cave_lamp"}
402+
assert result["total_entities"] == 2
379403
assert any("expanded to 2 area(s)" in warning for warning in result["warnings"])
380404

405+
@pytest.mark.asyncio
406+
async def test_bare_floor_id_expands_to_all_areas_on_floor(self):
407+
"""A literal floor_id expands the same way the floor's name does.
408+
409+
The collision test below renames an AREA to ``sous_sol``; nothing else
410+
covered passing the floor's own identifier while it stays unique.
411+
"""
412+
client = self._basement_client()
413+
client.areas.append({"area_id": "cave", "name": "Cave", "floor_id": "sous_sol"})
414+
tools = SmartSearchTools(client=client, fuzzy_threshold=60)
415+
416+
result = await tools.get_entities_by_area("sous_sol")
417+
418+
assert set(result["areas"]) == {"couloir_sous_sol", "cave"}
419+
assert any(
420+
"is a floor, not an area" in warning for warning in result["warnings"]
421+
)
422+
381423
@pytest.mark.asyncio
382424
async def test_floor_alias_expands_to_areas_on_floor(self):
383425
"""An exact floor alias follows the same deterministic expansion."""

0 commit comments

Comments
 (0)