Skip to content

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

Merged
eman merged 2 commits into
mainfrom
fix/evaluation-hotfixes
Jul 5, 2026
Merged

fix: address high-priority evaluation findings (hotfix batch)#90
eman merged 2 commits into
mainfrom
fix/evaluation-hotfixes

Conversation

@eman

@eman eman commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Fixes the highest-priority confirmed bugs from a comprehensive repo evaluation.

Changes

  • mqtt/client.py: configure_reservation_water_program() referenced self._control, which is never assigned (the attribute is _device_controller), so every call raised AttributeError. Fixed the delegation and added a regression test that scans the module for references to the undefined attribute.
  • CLI mode command: removed the vacation choice (mode 5 requires a day count that was never supplied, so it failed 100% of the time; use vacation DAYS) and the standby choice (0 is not a writable DhwOperationSetting value; use power off). Docs updated to match.
  • CLI exit codes: click ignores callback return values in standalone mode, so the CLI always exited 0 even on failure. Failures now propagate via ctx.exit() and produce a non-zero exit code.
  • Token cache security: ~/.nwp500_tokens.json (refresh token + AWS credentials) was written world-readable. Now created with mode 0600; existing files are tightened on save.
  • Wrong-account token reuse: 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 on email mismatch.
  • OpenEI error masking: OpenEI reports application errors (e.g. invalid API key) in the body of an HTTP 200 response; these were reported as "no rate plans found". fetch_rates() now raises APIError with the API's message.
  • Broken example: examples/advanced/mqtt_diagnostics.py imported from the nonexistent nwp500.mqtt_utils module and used deprecated datetime.utcnow().

Tests

  • 17 new tests, including a new tests/test_token_storage.py (file permissions, roundtrip)
  • Added an autouse fixture in test_cli_basic.py restoring logger levels mutated by the CLI group callback (previously leaked into test_utils when CLI extras are installed)

Validation

  • pytest: 520 passed (CLI tests run with [cli] extras installed)
  • ruff check / ruff format --check: clean
  • pyright src/nwp500: 0 errors
  • mypy src/nwp500: no issues

- Fix AttributeError in configure_reservation_water_program proxy
  (referenced nonexistent self._control instead of _device_controller)
- Remove broken 'vacation' and 'standby' choices from the CLI mode
  command; vacation requires a day count (use 'vacation DAYS') and
  standby (0) is not a writable operation mode (use 'power off')
- Propagate CLI failures as non-zero exit codes (click standalone mode
  ignores callback return values)
- Discard cached tokens when --email differs from the cached account
- Write ~/.nwp500_tokens.json with 0600 permissions and tighten
  existing files on save
- Raise APIError for OpenEI application errors returned in HTTP 200
  bodies instead of reporting 'no rate plans found'
- Fix broken import and deprecated datetime.utcnow() in
  examples/advanced/mqtt_diagnostics.py
- Update CLI mode docs to match the supported choices

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR delivers a batch of high-priority hotfixes across the MQTT client, CLI behavior, token-cache security, and OpenEI error handling, with regression tests and documentation updates to prevent recurrence.

Changes:

  • Fix NavienMqttClient.configure_reservation_water_program() to delegate to the correct controller attribute and add regression coverage.
  • Harden CLI behavior (remove unsupported mode choices, ensure non-zero exit codes on failure, and prevent cached-token reuse across different emails).
  • Improve reliability/security and correctness via token file permission tightening and raising APIError for OpenEI “HTTP 200 with error body” responses; update docs/examples accordingly.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_token_storage.py Adds token cache permission + roundtrip tests for CLI token storage.
tests/test_openei.py Adds a regression test ensuring OpenEI application errors raise APIError.
tests/test_mqtt_client_init.py Adds regression tests for MQTT client → device controller proxy delegation.
tests/test_cli_commands.py Adds coverage ensuring removed mode values aren’t handled by the handler.
tests/test_cli_basic.py Adds CLI behavior tests (invalid mode choices, non-zero exit codes, email mismatch behavior) plus logger-level isolation fixture.
src/nwp500/openei.py Raises APIError when OpenEI returns an "error" object in a 200 response body.
src/nwp500/mqtt/client.py Fixes delegation bug by using _device_controller instead of nonexistent _control.
src/nwp500/cli/token_storage.py Writes token cache file with restrictive permissions (0600), including tightening existing files.
src/nwp500/cli/handlers.py Removes unsupported standby/vacation from mode mapping and clarifies intent.
src/nwp500/cli/main.py Discards cached tokens on email mismatch; uses ctx.exit() to propagate non-zero exit codes; removes unsupported CLI choices.
examples/advanced/mqtt_diagnostics.py Fixes imports and replaces deprecated datetime.utcnow() usage.
docs/reference/python_api/cli.rst Updates CLI docs to reflect removed mode choices and directs users to correct commands.
CHANGELOG.rst Documents bugfixes and the token-cache permission security fix.

Comment thread tests/test_mqtt_client_init.py Outdated
Addresses review feedback: a raw substring search could false-fail on
comments, docstrings, or examples. The AST walk only flags actual
self._control attribute access.
@eman
eman merged commit 6c928a9 into main Jul 5, 2026
5 checks passed
@eman
eman deleted the fix/evaluation-hotfixes branch July 5, 2026 16:27
eman added a commit that referenced this pull request Jul 5, 2026
* fix: device protocol correctness

- Send flat raw protocol payloads for weekly reservation and
  recirculation schedules; the model dump double-nested the request
  and leaked computed display fields (incl. unit-converted temperature)
  into device commands. Add NavienBaseModel.to_protocol_dict()
- Preserve command queue order on failed flush (deque with re-insert
  at front instead of tail)
- Discard queued commands older than max_queued_command_age (default
  300s) instead of replaying stale control commands after long outages
- Use truncated remainder (firmware semantics) instead of Python's
  floored modulo in the ASYMMETRIC Fahrenheit formula; fixes
  off-by-one conversions for sub-zero temperatures
- Correct freeze protection default limits to raw half-Celsius 12/20
  (43F/50F documented fixed range) instead of 43/65
- Emit error_detected when the error code changes between two
  non-zero codes
- Round TOU prices half-up via Decimal (round() banker's rounding
  under-encoded exact halves); encode bool prices instead of passing
  them through as pre-encoded ints
- Fix inverted enable-flag doc, wrong week-bitfield example (158->84),
  and int/float doctest values

* refactor!: trim public API, remove dead code, deduplicate conversions

BREAKING CHANGES (major version bump required):

- Remove deprecated NavienMqttClient.control property; use the
  delegated methods on the client directly
- Remove never-raised exception classes: TokenExpiredError,
  MqttSubscriptionError, DeviceNotFoundError, DeviceOfflineError,
  DeviceOperationError (catch the parent classes instead)
- Stop re-exporting internal plumbing from the top-level namespace:
  requires_capability, MqttDeviceInfoCache,
  MqttDeviceCapabilityChecker, log_performance, and the eight
  bit-encoding helpers (import from nwp500.encoding et al.)

Cleanup:

- Delete dead nwp500.cli.commands module (never imported, drifted
  from the real click commands)
- Delete unused converters.str_enum_validator and module-level
  temperature conversion wrappers; simplify div_10/mul_10
- Delete no-op NavienMqttClient._on_message_received placeholder and
  unused CLI width calculations
- Deduplicate the four Temperature classes via a _scale ClassVar on
  the base (behavior unchanged, ~200 lines removed); doctests pass
- Deduplicate field_factory metadata-merge boilerplate
- Use converters.device_bool_from_python in mqtt/control.py instead
  of inline 2/1 literals
- Resolve auth.py version from distribution metadata, removing the
  order-dependent 'from . import __version__' near-circular import
- Update docs and examples to import encoding helpers from
  nwp500.encoding; add public-API surface tests

* chore: Python 3.14 modernization and lint rule adoption

- Remove 'from __future__ import annotations' project-wide (57 files);
  redundant on a >=3.14-only package (PEP 649 default)
- Adopt RUF, DTZ, PTH, ASYNC, PERF ruff rule sets with documented
  ignores (ASYNC109 public timeout params, RUF001-003 intentional
  unicode, RUF022 grouped __all__)
- Fix all resulting findings: timezone-aware CSV timestamps,
  Path.open(), itertools.pairwise in the CLI range collapser, ClassVar
  on the capability map, stored task references in tests
- Convert PeriodicRequestType to StrEnum
- Add slots=True to frozen event dataclasses (mqtt_events,
  EventListener)
- Use match for periodic request-type dispatch
- Write token cache with 0600 permissions (mirrors #90 so the stacked
  branches merge cleanly); fix deprecated datetime.utcnow() in the
  diagnostics example likewise
- Add unit tests for previously untested modules: topic_builder,
  field_factory, models/_converters (19 tests)

* fix: use Path.open() in diagnostics example; lint fixes after merge

Addresses review feedback: two open() calls on Path objects remained
in examples/advanced/mqtt_diagnostics.py.

---------

Co-authored-by: emmanuel <emmanuel@musta.lan>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants