fix(tools_integrations): address kp13 review subset (8 of 10 + 4 NITs)

Addresses 8 of kp13's 10 numbered review items + all 4 while-you're-in
bullets on PR #1424. Items 1 and 6 (Path 4 happy-path schema
harmonization and IDEMPOTENT-SUCCESS CONTRACT docstring rewrite) are
held pending the design-clarification response in issue-comment
4527825242.

Contract / schema:
- Drop the ``result`` passthrough key from Path 4 happy-path return
  (no other path returns it). [Item 2]

Docstring / contract accuracy:
- Field descriptions for ``target``, ``helper_type``, ``subentry_id``,
  ``confirm`` switched from "delete"/"deletion" to "remove"/"removal"
  to match the public tool verb. [Item 7]
- ``uniform for all three paths`` updated to ``four`` in both confirm
  and empty/whitespace gates; Path 4 added to the empty-target gate's
  enumeration. [Item 8]
- Stale ``ha_delete_helpers_integrations`` reference in
  docs/superpowers/specs/2026-05-21-1288-auto-backup-design.md updated.
  [Item 9]

Error handling:
- Narrowed ``_get_entry_id_for_flow_helper`` broad ``except Exception``
  to ``(OSError, TimeoutError)``. KeyError/AttributeError/TypeError now
  propagate so programmer bugs surface instead of being mis-classified
  as ``WEBSOCKET_DISCONNECTED``. [Item 10]

Tests:
- Replaced the tautological OR-disjunct in
  test_simple_path_non_404_apierror_propagates with two independent
  assertions so a regression that leaks ``already_deleted`` into a
  non-``ENTITY_NOT_FOUND`` code still fails. [Item 5a]
- Pass ``status_code=404`` to ``HomeAssistantAPIError`` in
  test_simple_path_disabled_state_check_apierror_resolves_via_registry
  — the test now actually exercises the 404 narrow it claims to cover.
  [Item 5b]
- New test_direct_path_non_404_apierror_surfaces_structured_error pins
  the Path 3 narrow (non-404 surfaces as structured error). [Item 4]
- New test_remove_helpers_integrations_subentry_string_error_surfaces_
  service_call_failed pins the Path 4 dict-only narrow against
  string-form ``"error": "not found"`` regressions. [Item 3]

While you're in there (Boy-Scout):
- Module comment near the private ``_delete_*`` helpers explaining the
  prefix-asymmetry (HA backend verb vs public ``remove`` family).
- Tightened "three deletion kinds in four paths" phrasing.
- FLOW contract bullet now lists ``bare_id_not_supported`` as a third
  failure mode (raises ``ENTITY_NOT_FOUND``).
- ``state_gone`` block comment compressed to one note about the
  never-existed-target consequence of the APIError-404 branch.

76 affected unit tests pass locally; ruff clean on touched files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: Patch76

docs/superpowers/specs/2026-05-21-1288-auto-backup-design.md

@@ -231,7 +231,7 @@ which would restart HA. Layered defenses:
 `ha_set_area_or_floor`, `ha_remove_area_or_floor`,
 `ha_set_todo_item`, `ha_remove_todo_item`,
 `ha_set_entity`,
-`ha_set_integration_enabled`, `ha_delete_helpers_integrations`.
+`ha_set_integration_enabled`, `ha_remove_helpers_integrations`.
 
 **Explicitly NOT wrapped:** `ha_call_service`, `ha_call_event`, `ha_restart`, `ha_reload_core`, `ha_check_config`, `ha_eval_template`, `ha_delete_file`, `ha_remove_entity`, `ha_remove_device`, `ha_update_device`, `ha_install_mcp_tools`, `ha_hacs_*`, blueprint/import ops.
 

src/ha_mcp/tools/tools_integrations.py

@@ -141,7 +141,13 @@ async def _get_entry_id_for_flow_helper(
     except (HomeAssistantConnectionError, HomeAssistantAuthError):
         # Typed errors must reach the outer handler — do not swallow.
         raise
-    except Exception as e:
+    except (OSError, TimeoutError) as e:
+        # Network / transport errors from the WS layer (ConnectionError,
+        # BrokenPipeError, TimeoutError, …). Programmer-bug-shape
+        # exceptions (KeyError, AttributeError, TypeError) intentionally
+        # propagate — the response is shape-checked at the dict guard
+        # below, and a raise here would otherwise mask the bug as a
+        # transient WEBSOCKET_DISCONNECTED.
         logger.debug(f"entity_registry/get failed for {entity_id}: {e}")
         if warnings is not None:
             warnings.append(f"entity_registry/get failed for {entity_id}: {e}")
@@ -1131,7 +1137,7 @@ async def ha_remove_helpers_integrations(
             str,
             Field(
                 description=(
-                    "What to delete. One of: "
+                    "What to remove. One of: "
                     "(a) bare helper_id for SIMPLE helpers (requires helper_type), "
                     "e.g. 'my_button'; "
                     "(b) full entity_id (requires helper_type), "
@@ -1149,7 +1155,7 @@ async def ha_remove_helpers_integrations(
                 description=(
                     "Helper type. Required when target is a helper_id (bare) "
                     "or entity_id. Set to None when target is a config entry_id "
-                    "to delete any integration. Use 'config_subentry' to delete "
+                    "to remove any integration. Use 'config_subentry' to remove "
                     "a config subentry under target."
                 ),
                 default=None,
@@ -1159,7 +1165,7 @@ async def ha_remove_helpers_integrations(
             str | None,
             Field(
                 description=(
-                    "Config subentry ID to delete when helper_type='config_subentry'."
+                    "Config subentry ID to remove when helper_type='config_subentry'."
                 ),
                 default=None,
             ),
@@ -1168,7 +1174,7 @@ async def ha_remove_helpers_integrations(
             bool | str,
             Field(
                 description=(
-                    "Must be True to confirm deletion. Accepts bool or "
+                    "Must be True to confirm removal. Accepts bool or "
                     "string ('true'/'false'/'1'/'0'/'yes'/'no'/'on'/'off', "
                     "case-insensitive) for transport ergonomics."
                 ),
@@ -1191,9 +1197,9 @@ async def ha_remove_helpers_integrations(
     ) -> dict[str, Any]:
         """Remove a Home Assistant helper or integration config entry.
 
-        Combines simple-helper websocket deletion, config-entry deletion, and
-        config-subentry deletion under one entry point with four routing paths
-        driven by helper_type.
+        Unifies three backend removal mechanisms — simple-helper websocket
+        delete, config-entry delete, and config-subentry delete — behind one
+        entry point with four routing paths driven by helper_type.
 
         WHEN NOT TO USE:
         - Removing only an entity (without deleting its underlying helper or
@@ -1227,7 +1233,9 @@ async def ha_remove_helpers_integrations(
         - FLOW: entity_id not in registry (target was never there or
           already removed). YAML-configured helpers (no config entry
           backing) still raise ``RESOURCE_NOT_FOUND`` — that's a
-          call-shape conflict, not a missing target.
+          call-shape conflict, not a missing target. A bare helper_id
+          (no ``.``) on a FLOW target raises ``ENTITY_NOT_FOUND`` —
+          FLOW resolution needs a full entity_id.
         - Direct config entry (helper_type=None): backend returns HTTP 404.
         - Config subentry: backend returns a "not_found" error.
 
@@ -1261,7 +1269,7 @@ async def ha_remove_helpers_integrations(
         Use ha_search_entities() / ha_get_integration() to verify before
         removal. Cannot be undone.
         """
-        # === Confirm gate (uniform for all three paths) ===
+        # === Confirm gate (uniform for all four paths) ===
         confirm_bool = coerce_bool_param(confirm, "confirm", default=False)
         if not confirm_bool:
             raise_tool_error(
@@ -1279,14 +1287,15 @@ async def ha_remove_helpers_integrations(
                 )
             )
 
-        # === Empty/whitespace target gate (uniform for all three paths) ===
+        # === Empty/whitespace target gate (uniform for all four paths) ===
         # Empty/whitespace ``target`` would reach the destructive backend call
         # on every path: Path 1 (simple-helper websocket delete), Path 2
         # (flow-helper entity-resolution → entry_id delete), Path 3
-        # (_delete_direct_entry → client.delete_config_entry("")). Each path
-        # surfaces a different misleading error from HA. Reject up-front so
-        # the caller learns the identifier was unusable before any backend
-        # call.
+        # (_delete_direct_entry → client.delete_config_entry("")), Path 4
+        # (_delete_config_subentry → ws delete on empty parent entry_id).
+        # Each path surfaces a different misleading error from HA. Reject
+        # up-front so the caller learns the identifier was unusable before
+        # any backend call.
         validate_identifier_not_empty(
             target,
             "target",
@@ -1339,6 +1348,12 @@ async def ha_remove_helpers_integrations(
             )
         )
 
+    # Private helpers keep the ``_delete_*`` prefix because they wrap HA's
+    # own backend verb — the WebSocket API is ``<type>/delete`` and the
+    # REST API is HTTP DELETE. The public tool surface uses ``remove`` to
+    # join the ``ha_remove_*`` behavioural family; the prefix asymmetry is
+    # intentional and prevents future renames pulled by either side.
+
     # === Path 3: Direct config entry delete (any integration) ===
     async def _delete_direct_entry(self, entry_id: str) -> dict[str, Any]:
         """Delete a config entry directly via the REST delete API."""
@@ -1680,7 +1695,6 @@ async def _delete_config_subentry(
             "subentry_id": subentry_id,
             "method": "config_subentry_delete",
             "message": f"Successfully deleted config subentry: {subentry_id}",
-            "result": result.get("result"),
         }
 
     # === Path 1: SIMPLE helper delete via websocket ===
@@ -1806,16 +1820,13 @@ async def _delete_simple_helper(
                             )
                     return response
 
-                # Fallback strategy 2: already-deleted check. Confirm via the
-                # registry too — a disabled entity is missing from the state
-                # machine but still registry-resident, so state-absence alone
-                # is not enough to declare success.
-                #
-                # state_gone is True when get_entity_state returns falsy OR
-                # raises HomeAssistantAPIError (404 = entity absent from the
-                # state machine). Without the APIError branch a never-existed
-                # target would fall through to ENTITY_NOT_FOUND instead of
-                # the idempotent already_deleted return.
+                # Fallback strategy 2: already-deleted check. Confirm via
+                # the registry too — a disabled entity is state-absent but
+                # still registry-resident, so state-absence alone is not
+                # enough to declare success. The APIError-404 branch covers
+                # the never-existed-target case (HA returns 404 on
+                # get_entity_state for unknown entity_ids); without it the
+                # caller would see ENTITY_NOT_FOUND for a typo'd target.
                 state_gone = False
                 try:
                     final_state_check = await client.get_entity_state(entity_id)

tests/src/unit/test_config_subentries_folded.py

@@ -555,3 +555,31 @@ async def test_remove_helpers_integrations_subentry_other_error_surfaces_service
     error_data = json.loads(str(exc_info.value))
     assert error_data["error"]["code"] == "SERVICE_CALL_FAILED"
     assert "Insufficient permissions" in error_data["error"]["message"]
+
+
+async def test_remove_helpers_integrations_subentry_string_error_surfaces_service_call_failed(
+    mock_client,
+):
+    """Path 4 narrow stays narrow: the idempotent ``not_found`` branch
+    only fires when ``error`` is a dict carrying ``code="not_found"``.
+    A string-form ``"error": "not found"`` (no structured code) must
+    NOT classify as idempotent — it surfaces as SERVICE_CALL_FAILED.
+    Without this pin, a regression that loosened to substring-matching
+    against ``error_msg`` would pass CI silently.
+    """
+    mock_client.delete_config_subentry.return_value = {
+        "success": False,
+        "error": "Subentry not found",
+    }
+
+    with pytest.raises(ToolError) as exc_info:
+        await IntegrationTools(mock_client).ha_remove_helpers_integrations(
+            target="entry-1",
+            helper_type="config_subentry",
+            subentry_id="subentry-1",
+            confirm=True,
+        )
+
+    error_data = json.loads(str(exc_info.value))
+    assert error_data["error"]["code"] == "SERVICE_CALL_FAILED"
+    assert "already_deleted" not in json.dumps(error_data)

tests/src/unit/test_tools_integrations.py

@@ -208,6 +208,28 @@ async def test_direct_path_entry_not_found(self, tools, mock_client):
         assert result["entry_id"] == "ghost_entry"
         assert result["method"] == "config_entry_delete"
 
+    async def test_direct_path_non_404_apierror_surfaces_structured_error(
+        self, tools, mock_client
+    ):
+        """Pin the Path 3 narrow: only status_code == 404 routes to
+        idempotent already_deleted. Non-404 API errors (500 server, 401
+        auth, …) must propagate via exception_to_structured_error and
+        surface as a structured error, not as idempotent success. Mirrors
+        the Path 1 narrow in test_simple_path_non_404_apierror_propagates.
+        """
+        mock_client.delete_config_entry.side_effect = HomeAssistantAPIError(
+            "Internal server error", status_code=500
+        )
+        with pytest.raises(ToolError) as exc_info:
+            await tools.ha_remove_helpers_integrations(
+                target="some_entry",
+                confirm=True,
+            )
+        err = json.loads(str(exc_info.value))
+        assert err["success"] is False
+        assert "already_deleted" not in json.dumps(err)
+        assert err["error"]["code"] != "ENTITY_NOT_FOUND"
+
     # === Path 1: SIMPLE ===
 
     async def test_simple_path_standard_via_unique_id(self, tools, mock_client):
@@ -386,11 +408,13 @@ async def test_simple_path_non_404_apierror_propagates(self, tools, mock_client)
         err = json.loads(str(exc_info.value))
         assert err["success"] is False
         # Outer chain converts the propagated APIError via
-        # exception_to_structured_error — must NOT be classified as
-        # already_deleted.
-        assert err["error"]["code"] != "ENTITY_NOT_FOUND" or (
-            "already_deleted" not in err.get("error", {}).get("message", "")
-        )
+        # exception_to_structured_error — must surface as a structured
+        # error, not as the idempotent ENTITY_NOT_FOUND/already_deleted
+        # branch. Two independent assertions so a regression that leaks
+        # ``already_deleted`` into a non-``ENTITY_NOT_FOUND`` code still
+        # fails the test (the previous OR-disjunct passed in that case).
+        assert err["error"]["code"] != "ENTITY_NOT_FOUND"
+        assert "already_deleted" not in json.dumps(err)
 
     async def test_simple_path_disabled_no_unique_id_surfaces_error(
         self, tools, mock_client
@@ -438,7 +462,7 @@ async def test_simple_path_disabled_state_check_apierror_resolves_via_registry(
         still has to run.
         """
         mock_client.get_entity_state.side_effect = HomeAssistantAPIError(
-            "404 simulated"
+            "404 simulated", status_code=404
         )
         mock_client.send_websocket_message.side_effect = [
             {"success": True, "result": {"unique_id": "uid-disabled-apierror"}},