fix: address high-priority evaluation findings (hotfix batch)

CHANGELOG.rst

@@ -5,6 +5,40 @@ Changelog
 Unreleased
 ==========
 
+Bug Fixes
+---------
+- **Fix AttributeError in configure_reservation_water_program**: The
+  ``NavienMqttClient`` proxy referenced ``self._control``, which is never
+  assigned (the attribute is ``_device_controller``), so every call raised
+  ``AttributeError``. Now delegates correctly; a regression test guards all
+  proxies against references to the undefined attribute.
+- **Fix broken CLI mode choices**: ``nwp-cli mode vacation`` always failed
+  because vacation mode (5) requires a day count that was never supplied, and
+  ``mode standby`` sent the invalid writable mode value ``0``. Both choices
+  were removed from the ``mode`` command; use the dedicated ``vacation DAYS``
+  and ``power off`` commands instead.
+- **Fix CLI exit codes**: click ignores command return values in standalone
+  mode, so the CLI always exited ``0`` even when a command failed. Failures
+  now propagate through ``ctx.exit()`` and produce a non-zero exit code for
+  scripts and automation.
+- **Fix cached tokens being reused for a different account**: passing
+  ``--email`` for account B while tokens for account A were cached silently
+  ran commands against account A's session. Cached tokens are now discarded
+  when the provided email does not match the cached one.
+- **Surface OpenEI application errors**: OpenEI reports errors such as an
+  invalid API key in the body of an HTTP 200 response; these were masked as
+  "no rate plans found". ``fetch_rates()`` now raises ``APIError`` with the
+  API's error message.
+- **Fix broken example import**: ``examples/advanced/mqtt_diagnostics.py``
+  imported ``MqttConnectionConfig`` from the nonexistent ``nwp500.mqtt_utils``
+  module and used the deprecated ``datetime.utcnow()``.
+
+Security
+--------
+- **Restrict token cache file permissions**: ``~/.nwp500_tokens.json``
+  (refresh token and AWS credentials) was written world-readable. It is now
+  created with mode ``0600``, and existing files are tightened on save.
+
 Version 8.1.3 (2026-06-15)
 ==========================
 

docs/reference/python_api/cli.rst

@@ -224,12 +224,6 @@ Set operation mode.
    # High Demand (maximum capacity)
    python3 -m nwp500.cli mode high-demand
 
-   # Vacation Mode
-   python3 -m nwp500.cli mode vacation
-
-   # Standby
-   python3 -m nwp500.cli mode standby
-
 **Syntax:**
 
 .. code-block:: bash
@@ -238,12 +232,13 @@ Set operation mode.
 
 **Available Modes:**
 
-* ``standby`` - Device off but ready
-* ``heat-pump`` - Heat pump only (0)
+* ``heat-pump`` - Heat pump only (1)
 * ``electric`` - Electric heating only (2)
 * ``energy-saver`` - Hybrid/balanced mode (3) **recommended**
 * ``high-demand`` - Maximum heating capacity (4)
-* ``vacation`` - Extended vacancy mode (5)
+
+For vacation mode use the ``vacation`` command (it requires a day count);
+to power the unit off use the ``power`` command.
 
 **Output:** Confirmation message and updated device status.
 

examples/advanced/mqtt_diagnostics.py

@@ -27,8 +27,7 @@
 from pathlib import Path
 
 from nwp500 import NavienAuthClient, NavienMqttClient
-from nwp500.mqtt import MqttDiagnosticsCollector
-from nwp500.mqtt_utils import MqttConnectionConfig
+from nwp500.mqtt import MqttConnectionConfig, MqttDiagnosticsCollector
 
 # Configure logging to show detailed MQTT information
 logging.basicConfig(
@@ -275,7 +274,7 @@ async def run_example(
 
                     # Final export
                     _logger.info("Exporting final diagnostics...")
-                    timestamp = datetime.utcnow().strftime("%Y%m%d_%H%M%S")
+                    timestamp = datetime.now(UTC).strftime("%Y%m%d_%H%M%S")
                     final_file = self.output_dir / f"diagnostics_final_{timestamp}.json"
                     with open(final_file, "w") as f:
                         f.write(self.diagnostics.export_json())

src/nwp500/cli/__main__.py

@@ -88,6 +88,10 @@ async def runner() -> int:
 
             # Load cached tokens if available
             tokens, cached_email = load_tokens()
+            # Cached tokens belong to cached_email; never reuse them for a
+            # different account.
+            if email and cached_email and email != cached_email:
+                tokens = None
             # If email not provided in args, try cached email
             email = email or cached_email
 
@@ -162,7 +166,9 @@ async def runner() -> int:
                 _formatter.print_error(str(e), title="Unexpected Error")
             return 1
 
-        return asyncio.run(runner())
+        # click ignores callback return values in standalone mode, so
+        # propagate failures via ctx.exit to get a non-zero exit code.
+        ctx.exit(asyncio.run(runner()))
 
     return wrapper
 
@@ -288,12 +294,10 @@ async def power(mqtt: NavienMqttClient, device: Any, state: str) -> None:
     "mode_name",
     type=click.Choice(
         [
-            "standby",
             "heat-pump",
             "electric",
             "energy-saver",
             "high-demand",
-            "vacation",
         ],
         case_sensitive=False,
     ),

src/nwp500/cli/handlers.py

@@ -220,13 +220,13 @@ async def handle_set_mode_request(
     mqtt: NavienMqttClient, device: Device, mode_name: str
 ) -> None:
     """Set device operation mode."""
+    # Vacation mode (5) requires a day count and has its own `vacation`
+    # command; power-off (6) is handled by the `power` command.
     mode_mapping = {
-        "standby": 0,
         "heat-pump": 1,
         "electric": 2,
         "energy-saver": 3,
         "high-demand": 4,
-        "vacation": 5,
     }
     mode_id = mode_mapping.get(mode_name.lower())
     if mode_id is None:

src/nwp500/cli/token_storage.py

@@ -4,6 +4,7 @@
 
 import json
 import logging
+import os
 from pathlib import Path
 
 from nwp500.auth import AuthTokens
@@ -22,7 +23,11 @@ def save_tokens(tokens: AuthTokens, email: str) -> None:
         email: User email address
     """
     try:
-        with open(TOKEN_FILE, "w") as f:
+        # Tokens grant account access; keep the file owner-readable only.
+        # O_CREAT mode only applies to new files, so chmod existing ones.
+        fd = os.open(TOKEN_FILE, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+        with os.fdopen(fd, "w") as f:
+            TOKEN_FILE.chmod(0o600)
             # Use the built-in to_dict() method for serialization
             token_data = tokens.to_dict()
             token_data["email"] = email

src/nwp500/mqtt/client.py

@@ -1268,7 +1268,8 @@ async def update_weekly_reservation(
 
     async def configure_reservation_water_program(self, device: Device) -> int:
         """Enable/configure water program reservation mode."""
-        return await self._control.configure_reservation_water_program(device)
+        controller = self._device_controller
+        return await controller.configure_reservation_water_program(device)
 
     async def configure_recirculation_schedule(
         self, device: Device, schedule: RecirculationSchedule

src/nwp500/openei.py

@@ -16,6 +16,8 @@
 
 import aiohttp
 
+from .exceptions import APIError
+
 __author__ = "Emmanuel Levijarvi"
 __copyright__ = "Emmanuel Levijarvi"
 __license__ = "MIT"
@@ -120,6 +122,16 @@ async def fetch_rates(
         async with self._session.get(OPENEI_API_URL, params=params) as resp:
             resp.raise_for_status()
             data: dict[str, Any] = await resp.json()
+            # OpenEI reports application errors (e.g. invalid API key) in
+            # the body of an HTTP 200 response.
+            if "error" in data:
+                error = data["error"]
+                message = (
+                    error.get("message", str(error))
+                    if isinstance(error, dict)
+                    else str(error)
+                )
+                raise APIError(f"OpenEI API error: {message}", response=data)
             items: list[dict[str, Any]] = data.get("items", [])
             _logger.info(
                 "Retrieved %d rate plans for zip %s",

tests/test_cli_basic.py

@@ -1,5 +1,6 @@
 """Basic tests for CLI entry point."""
 
+import logging
 import sys
 from unittest.mock import patch
 
@@ -11,6 +12,16 @@
     pytest.skip("CLI dependencies not installed", allow_module_level=True)
 
 
+@pytest.fixture(autouse=True)
+def _restore_log_levels():
+    """The CLI group callback mutates global logger levels; undo it."""
+    names = (None, "nwp500", "nwp500.cli.__main__", "aiohttp")
+    saved = {name: logging.getLogger(name).level for name in names}
+    yield
+    for name, level in saved.items():
+        logging.getLogger(name).setLevel(level)
+
+
 def test_cli_help():
     """Test that CLI help command works."""
     with patch.object(sys, "argv", ["nwp-cli", "--help"]):
@@ -25,3 +36,76 @@ def test_cli_no_args():
         with pytest.raises(SystemExit) as excinfo:
             run()
         assert excinfo.value.code != 0
+
+
+def test_cli_mode_rejects_vacation():
+    """Vacation is not a valid mode argument (use the vacation command)."""
+    with patch.object(sys, "argv", ["nwp-cli", "mode", "vacation"]):
+        with pytest.raises(SystemExit) as excinfo:
+            run()
+        assert excinfo.value.code == 2  # click usage error
+
+
+def test_cli_mode_rejects_standby():
+    """Standby (0) is not a writable operation mode."""
+    with patch.object(sys, "argv", ["nwp-cli", "mode", "standby"]):
+        with pytest.raises(SystemExit) as excinfo:
+            run()
+        assert excinfo.value.code == 2  # click usage error
+
+
+def test_cli_exits_nonzero_on_missing_credentials():
+    """Command failures must produce a non-zero exit code."""
+    import os
+
+    env = {
+        k: v
+        for k, v in os.environ.items()
+        if k not in ("NAVIEN_EMAIL", "NAVIEN_PASSWORD")
+    }
+    with (
+        patch.dict(os.environ, env, clear=True),
+        patch("nwp500.cli.__main__.load_tokens", return_value=(None, None)),
+        patch.object(sys, "argv", ["nwp-cli", "mode", "heat-pump"]),
+    ):
+        with pytest.raises(SystemExit) as excinfo:
+            run()
+        assert excinfo.value.code == 1
+
+
+def test_cli_discards_cached_tokens_for_different_email():
+    """Cached tokens for account A must not be reused for account B."""
+    from unittest.mock import MagicMock
+
+    from nwp500.exceptions import AuthenticationError
+
+    cached_tokens = MagicMock()
+    mock_auth_cls = MagicMock(
+        side_effect=AuthenticationError("stop before network")
+    )
+    with (
+        patch(
+            "nwp500.cli.__main__.load_tokens",
+            return_value=(cached_tokens, "usera@example.com"),
+        ),
+        patch("nwp500.cli.__main__.NavienAuthClient", mock_auth_cls),
+        patch.object(
+            sys,
+            "argv",
+            [
+                "nwp-cli",
+                "--email",
+                "userb@example.com",
+                "--password",
+                "pw",
+                "mode",
+                "heat-pump",
+            ],
+        ),
+    ):
+        with pytest.raises(SystemExit) as excinfo:
+            run()
+        assert excinfo.value.code == 1
+    mock_auth_cls.assert_called_once_with(
+        "userb@example.com", "pw", stored_tokens=None
+    )

tests/test_cli_commands.py

@@ -131,6 +131,21 @@ async def test_handle_set_mode_request_invalid_mode(mock_mqtt, mock_device):
     mock_mqtt.set_dhw_mode.assert_not_called()
 
 
+@pytest.mark.asyncio
+@pytest.mark.parametrize("mode_name", ["standby", "vacation"])
+async def test_handle_set_mode_request_unsupported_modes(
+    mock_mqtt, mock_device, mode_name
+):
+    """Standby/vacation are not settable via the mode command.
+
+    Vacation requires a day count (dedicated ``vacation`` command) and
+    standby (0) is not a writable DhwOperationSetting value.
+    """
+    await handle_set_mode_request(mock_mqtt, mock_device, mode_name)
+
+    mock_mqtt.set_dhw_mode.assert_not_called()
+
+
 @pytest.mark.asyncio
 async def test_handle_set_dhw_temp_request_success(mock_mqtt, mock_device):
     """Test successful temperature setting."""

tests/test_mqtt_client_init.py

@@ -830,3 +830,52 @@ async def mock_connect():
                 # Verify call order
                 assert call_order == ["ensure_valid_token", "connect"]
                 assert result is True
+
+
+class TestDeviceControllerProxies:
+    """Regression tests for client -> device-controller delegation."""
+
+    @pytest.mark.asyncio
+    async def test_configure_reservation_water_program_delegates(
+        self, auth_client_with_valid_tokens
+    ):
+        """Regression: this proxy referenced a nonexistent self._control."""
+        from unittest.mock import AsyncMock, MagicMock
+
+        mqtt_client = NavienMqttClient(auth_client_with_valid_tokens)
+        mqtt_client._device_controller = AsyncMock()
+        mqtt_client._device_controller.configure_reservation_water_program = (
+            AsyncMock(return_value=1)
+        )
+        device = MagicMock()
+
+        result = await mqtt_client.configure_reservation_water_program(device)
+
+        assert result == 1
+        proxy_target = mqtt_client._device_controller
+        proxy_target.configure_reservation_water_program.assert_awaited_once_with(
+            device
+        )
+
+    def test_no_references_to_undefined_control_attribute(self):
+        """No code may reference self._control (never assigned).
+
+        Uses an AST walk rather than a source substring search so that
+        mentions in comments, docstrings, or examples cannot cause
+        false failures — only actual attribute access counts.
+        """
+        import ast
+        import inspect
+
+        import nwp500.mqtt.client as client_module
+
+        tree = ast.parse(inspect.getsource(client_module))
+        offenders = [
+            node.lineno
+            for node in ast.walk(tree)
+            if isinstance(node, ast.Attribute)
+            and node.attr == "_control"
+            and isinstance(node.value, ast.Name)
+            and node.value.id == "self"
+        ]
+        assert offenders == []