Fix: Bugs and Improvements from Code Review by eman · Pull Request #73 · eman/nwp500-python

eman · 2026-02-13T03:51:32Z

This PR addresses several issues identified in the code review (reference/Code Review_ Bugs and Improvements.md).

Fixed Critical Bug: Added resubscribe_all() call during active reconnection in MQTT client.
Fixed Resource Leak: Implemented proper unsubscription in ensure_device_info_cached using new unsubscribe_device_feature method.
Fixed Documentation: Removed hardcoded °F unit in README example.
Improved API Client: Exposed session property in NavienAuthClient and updated NavienAPIClient to use it.
Fixed Data Model: Removed static unit from recirc_dhw_flow_rate to support dynamic units.

Validation:

make ci-lint passed.
pytest passed (402 tests).

Addressed the following issues: 1. Fixed lost subscriptions during active reconnect in mqtt/client.py 2. Fixed resource leak in ensure_device_info_cached by adding unsubscribe_device_feature 3. Fixed misleading units in README.rst example 4. Exposed session property in NavienAuthClient and used it in NavienAPIClient 5. Removed static unit from recirc_dhw_flow_rate in models.py

Addressed 4 high severity alerts regarding clear-text logging of sensitive information (topics). - Fixed direct logging of topic in subscriptions.py - Refactored redact_topic to use separate variable for better static analysis - Added redaction to debug logs

src/nwp500/mqtt/subscriptions.py

…sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Corrected direct topic logging in subscriptions.py that was missed or incorrectly edited in previous attempt. Now consistently using redact_topic(topic) and fixed line length.

src/nwp500/mqtt/subscriptions.py

Ran 'tox -e lint' locally and fixed formatting issues in subscriptions.py caught by ruff format check.

src/nwp500/mqtt/subscriptions.py

…sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Replaced specific topic logging with generic messages in unsubscription logic to prevent potential sensitive data leakage and satisfy CodeQL security checks.

Applied ruff format to fix linting errors introduced by previous CodeQL fix.

Copilot

Pull request overview

This PR addresses multiple issues identified in a code review, focusing on fixing critical bugs, resource leaks, and improving code quality across the MQTT client, API client, and documentation.

Changes:

Fixed critical MQTT reconnection bug by adding resubscribe_all() call after active reconnection
Fixed resource leak by implementing proper unsubscription mechanism for device feature callbacks
Improved logging consistency by redacting sensitive MQTT topics
Enhanced API client to use the new public session property from auth client
Fixed data model to remove static unit specification for dynamic flow rate field

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/nwp500/mqtt/client.py	Added `resubscribe_all()` call during active reconnection and implemented `unsubscribe_device_feature()` method
src/nwp500/mqtt/subscriptions.py	Added topic redaction in logging, implemented `unsubscribe_device_feature()` with handler tracking via `_original_callback`
src/nwp500/mqtt/utils.py	Introduced intermediate variable for topic redaction to improve clarity
src/nwp500/auth.py	Exposed `session` property for public access
src/nwp500/api_client.py	Updated to use new `session` property with fallback for compatibility
src/nwp500/models.py	Removed hardcoded unit from `recirc_dhw_flow_rate` to support dynamic units
README.rst	Removed hardcoded temperature unit from example to reflect dynamic unit support

Copilot · 2026-02-13T04:47:52Z

src/nwp500/mqtt/subscriptions.py

            self._message_handlers.pop(topic, None)

-            _logger.info(f"Unsubscribed from '{topic}'")
+            _logger.info("Unsubscribed from topic (redacted)")


For consistency with line 235, this should use the redacted topic format: _logger.info(f"Unsubscribed from topic: {redact_topic(topic)}") instead of a generic message. This provides better debugging information while still protecting sensitive data.

Suggested change

_logger.info("Unsubscribed from topic (redacted)")

_logger.info(f"Unsubscribed from topic: {redact_topic(topic)}")

Copilot · 2026-02-13T04:47:52Z

src/nwp500/mqtt/subscriptions.py

-            _logger.error(
-                f"Failed to unsubscribe from '{redact_topic(topic)}': {e}"
-            )
+            _logger.error(f"Failed to unsubscribe from topic (redacted): {e}")


For consistency with line 235, this should use the redacted topic format: _logger.error(f"Failed to unsubscribe from topic: {redact_topic(topic)}: {e}") instead of a generic message. The redact_topic function is already imported and used elsewhere in this file.

Suggested change

_logger.error(f"Failed to unsubscribe from topic (redacted): {e}")

_logger.error(

f"Failed to unsubscribe from topic: {redact_topic(topic)}: {e}"

)

Copilot · 2026-02-13T04:47:52Z

src/nwp500/mqtt/subscriptions.py

+                    "Unsubscribe from topic (redacted) was "
                    "cancelled but will complete in background"


For consistency with line 235, this should use the redacted topic format: "Unsubscribe from topic: {redact_topic(topic)} was cancelled but will complete in background" instead of a generic message.

Suggested change

"Unsubscribe from topic (redacted) was "

"cancelled but will complete in background"

"Unsubscribe from topic: %s was cancelled but will complete in background",

redact_topic(topic),

Copilot · 2026-02-13T04:47:53Z

src/nwp500/api_client.py

+        # Use public session property if available, fall back to private
+        # _session for backward compatibility or if using older auth client
+        auth_session = getattr(auth_client, "session", None)
+        if auth_session is None:
+            auth_session = getattr(auth_client, "_session", None)
+
+        self._session = session or auth_session


The comment mentions "backward compatibility or if using older auth client", but since the session property is being added in this same PR (auth.py lines 636-639), there are no "older auth clients" that lack this property yet. This fallback logic is unnecessary. Consider simplifying to just self._session = session or auth_client.session since the property will always be available.

Suggested change

# Use public session property if available, fall back to private

# _session for backward compatibility or if using older auth client

auth_session = getattr(auth_client, "session", None)

if auth_session is None:

auth_session = getattr(auth_client, "_session", None)

self._session = session or auth_session

self._session = session or auth_client.session

Copilot · 2026-02-13T04:47:53Z

src/nwp500/mqtt/utils.py

+    # Use a new variable to avoid confusion in static analysis
+    redacted_topic = topic


The introduction of the redacted_topic variable is unnecessary and doesn't solve any actual static analysis issues. The function should directly reassign to topic as it did before. This adds complexity without benefit and makes the code less idiomatic. The comment mentions "confusion in static analysis" but there's no evidence of any such issue, and reassigning parameters is a common Python pattern.

Copilot · 2026-02-13T04:47:53Z

src/nwp500/mqtt/subscriptions.py

+        # Redact topic for logging to avoid leaking sensitive information
+        # (device IDs). We perform this check early to ensure we don't log raw
+        # topics.
+        # Note: CodeQL may still flag this if we log the variable derived from
+        # topic, so we use a generic message.
+        _logger.info("Unsubscribing from topic (redacted)")


This logging approach is inconsistent with the subscribe method at line 171, which logs "Subscribing to topic: {redact_topic(topic)}" showing the redacted topic. For consistency and better diagnostics, consider using _logger.info(f"Unsubscribing from topic: {redact_topic(topic)}") instead of a completely generic message. This maintains useful debugging information while still protecting sensitive data.

Incorporated feedback from PR #73: - api_client.py: Simplified session property fallback logic. - mqtt/utils.py: Reverted unnecessary variable introduction in redact_topic. - mqtt/subscriptions.py: Restored consistent logging format with topic redaction while maintaining security posture.

src/nwp500/mqtt/subscriptions.py

Resolved CodeQL 'Clear-text logging of sensitive information' alerts by using generic log messages for sensitive topics, as taint analysis flagged redaction as insufficient. Also fixed formatting issues to pass lint checks.

eman added 2 commits February 12, 2026 19:50

github-advanced-security bot found potential problems Feb 13, 2026

View reviewed changes

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

eman and others added 2 commits February 12, 2026 20:04

Potential fix for code scanning alert no. 157: Clear-text logging of …

6bbcaf1

…sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Fix remaining CodeQL alerts for topic logging

427502b

Corrected direct topic logging in subscriptions.py that was missed or incorrectly edited in previous attempt. Now consistently using redact_topic(topic) and fixed line length.

github-advanced-security bot found potential problems Feb 13, 2026

View reviewed changes

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

Fix formatting validation failure in CI

54eac55

Ran 'tox -e lint' locally and fixed formatting issues in subscriptions.py caught by ruff format check.

github-advanced-security bot found potential problems Feb 13, 2026

View reviewed changes

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

eman and others added 3 commits February 12, 2026 20:29

Potential fix for code scanning alert no. 162: Clear-text logging of …

11dda6b

…sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Fix CodeQL alerts by removing sensitive topic logging

237b3a8

Replaced specific topic logging with generic messages in unsubscription logic to prevent potential sensitive data leakage and satisfy CodeQL security checks.

Fix formatting in subscriptions.py

f1eb41b

Applied ruff format to fix linting errors introduced by previous CodeQL fix.

eman requested a review from Copilot February 13, 2026 04:42

Copilot started reviewing on behalf of eman February 13, 2026 04:43 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

github-advanced-security bot found potential problems Feb 13, 2026

View reviewed changes

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

src/nwp500/mqtt/subscriptions.py Fixed Show fixed Hide fixed

Fix CI failures (CodeQL + Formatting)

e331d70

Resolved CodeQL 'Clear-text logging of sensitive information' alerts by using generic log messages for sensitive topics, as taint analysis flagged redaction as insufficient. Also fixed formatting issues to pass lint checks.

eman merged commit c72cd64 into main Feb 13, 2026
7 checks passed

eman deleted the fix-bugs-improvements branch February 13, 2026 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Bugs and Improvements from Code Review#73

Fix: Bugs and Improvements from Code Review#73
eman merged 10 commits intomainfrom
fix-bugs-improvements

eman commented Feb 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	_logger.info("Unsubscribed from topic (redacted)")
	_logger.info(f"Unsubscribed from topic: {redact_topic(topic)}")

		"Unsubscribe from topic (redacted) was "
		"cancelled but will complete in background"

		# Use a new variable to avoid confusion in static analysis
		redacted_topic = topic

Conversation

eman commented Feb 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants