docs(tests): correct the e2e expected-failure helper guidance

tests/AGENTS.md

@@ -49,8 +49,16 @@ rather than assume the e2e's always-present case.
 ## Test Patterns
 
 - Tests expecting tool **success**: use `mcp.call_tool_success()` inside `MCPAssertions` context
-- Tests expecting tool **failure**: use `safe_call_tool()` directly (catches `ToolError`, returns parsed dict)
-- Service availability checks should use `safe_call_tool` to probe, not `call_tool_success`
+- Tests expecting tool **failure**: use `mcp.call_tool_failure()` inside `MCPAssertions`
+  context. It rejects any result `assert_mcp_success()` would accept — including the
+  tools that succeed with no `success` key (`pending_restart`, bulk-operation payloads)
+  — so it genuinely proves the call failed. Prefer also passing `expected_error` to pin
+  *which* failure; about half the current call sites omit it and assert on the returned
+  dict themselves instead, which is equally fine.
+- `safe_call_tool()` is for calls whose outcome the test does **not** assert: `finally`
+  cleanup (so a cleanup failure cannot mask the real assertion) and service-availability
+  probes. It swallows `ToolError` and returns a parsed dict, so using it for an expected
+  failure means nothing verifies the call failed at all.
 
 ## E2E Test Patterns
 

tests/src/e2e/utilities/assertions.py

@@ -113,8 +113,11 @@ async def safe_call_tool(
 ) -> dict[str, Any]:
     """Call an MCP tool and return parsed result, handling ToolError exceptions.
 
-    This is useful for tests that expect tools to fail and want to inspect
-    the error response without catching exceptions manually.
+    For a call whose outcome the test does NOT assert: ``finally``-block cleanup
+    (so a cleanup failure cannot mask the real assertion) and service-availability
+    probes. It swallows ``ToolError`` and returns a parsed dict either way, so a
+    test that asserts a failure should use ``MCPAssertions.call_tool_failure()``
+    with ``expected_error`` instead -- see tests/AGENTS.md "Test Patterns".
 
     Args:
         mcp_client: The MCP client instance
@@ -131,17 +134,16 @@ async def safe_call_tool(
         return tool_error_to_result(exc)
 
 
-def assert_mcp_success(result, operation_name: str = "operation"):
-    """
-    Assert that MCP tool result indicates success.
+def looks_like_success(data: dict[str, Any]) -> bool:
+    """Whether a parsed tool result is a success response.
 
-    Args:
-        result: FastMCP client result
-        operation_name: Name of operation for error message
+    Shared by ``assert_mcp_success`` and ``assert_mcp_failure`` so the two
+    cannot disagree about what "success" means. Several tools succeed
+    WITHOUT a ``success`` key (``pending_restart``, bulk-operation
+    payloads), so a bare ``data.get("success")`` check treats those as
+    failures -- which is fine for the success assertion (it lists them
+    explicitly) but silently accepted them as failures on the other side.
     """
-    data = parse_mcp_result(result)
-
-    # Handle different success indicators
     success_indicators = [
         data.get("success") is True,
         # ha_manage_app's options/network write returns
@@ -172,7 +174,20 @@ def assert_mcp_success(result, operation_name: str = "operation"):
         ),
     ]
 
-    if not any(success_indicators):
+    return any(success_indicators)
+
+
+def assert_mcp_success(result, operation_name: str = "operation"):
+    """
+    Assert that MCP tool result indicates success.
+
+    Args:
+        result: FastMCP client result
+        operation_name: Name of operation for error message
+    """
+    data = parse_mcp_result(result)
+
+    if not looks_like_success(data):
         error_msg = data.get("error", "Unknown error")
         suggestions = data.get("suggestions", [])
 
@@ -199,8 +214,12 @@ def assert_mcp_failure(
     """
     data = parse_mcp_result(result)
 
-    # Check that operation actually failed
-    if data.get("success"):
+    # Check that operation actually failed. Uses the shared success
+    # predicate, not a bare data.get("success"): a tool that succeeds
+    # without a success key (pending_restart, bulk-operation payloads)
+    # would otherwise be accepted here as a failure, so a regression that
+    # made an expected-failure call SUCCEED could pass unnoticed.
+    if looks_like_success(data):
         raise AssertionError(f"{operation_name} should have failed but succeeded")
 
     # If expected error specified, check for it
@@ -380,8 +399,9 @@ async def call_tool_failure(
         except ToolError as exc:
             # Convert ToolError to result dict and validate
             data = tool_error_to_result(exc)
-            # Verify this is actually a failure
-            if data.get("success"):
+            # Verify this is actually a failure (shared predicate, see
+            # assert_mcp_failure)
+            if looks_like_success(data):
                 raise AssertionError(
                     f"{operation_name} should have failed but succeeded"
                 ) from exc