refactor: modularize OpenEVSE client by splitting core logic into dedicated modules and updating test suite#541
Open
refactor: modularize OpenEVSE client by splitting core logic into dedicated modules and updating test suite#541
Conversation
…icated modules and updating test suite
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMonolithic OpenEVSE logic was extracted into a dedicated async client and feature modules; package exports and main were converted to re-exports; new Requester, Firmware, Override, Limit, Claims, and Sensors modules were added; websocket scheduling was adjusted; Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OpenEVSE (client.py)
participant Requester as Requester (requester.py)
participant Firmware as Firmware (firmware.py)
participant Server as EVSE HTTP API
participant WS as WebSocket Listener
Client->>Requester: process_request(url, method, data)
Requester->>Server: HTTP request
Server-->>Requester: HTTP response (JSON / status)
Requester-->>Client: parsed response / error
Client->>WS: ws_start()
WS->>Server: ws_connect()
Server-->>WS: websocket messages (status updates)
WS-->>Client: _update_status(payload)
Client->>Client: merge status, check UPDATE_TRIGGERS
alt update triggers require firmware check
Client->>Firmware: firmware_check() / version_check()
Firmware-->>Client: version result
Client->>Requester: GET /status or GET /config (as needed)
end
Client->>Client: invoke user callback (sync or async)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
firstof9
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (2)
tests/test_client.py (1)
783-786: Move thecaplogassertions outside thepytest.raisesblocks.The awaited call is the statement that raises in both branches, so the
assert ... in caplog.textlines never run. That leaves the log behavior untested.Suggested fix
- with pytest.raises(UnsupportedFeature): - with caplog.at_level(logging.DEBUG): - await test_charger_unknown_semver.divert_mode() - assert "Non-semver firmware version detected." in caplog.text + with caplog.at_level(logging.DEBUG): + with pytest.raises(UnsupportedFeature): + await test_charger_unknown_semver.divert_mode() + assert "Non-semver firmware version detected." in caplog.textApply the same structure to the
UnknownErrorbranch intest_set_charge_mode().Also applies to: 1235-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 783 - 786, The test currently wraps both the awaited call and the caplog assertion inside pytest.raises, so the assertion on caplog.text never executes; change both occurrences to only await the raising call inside the pytest.raises context (e.g., await test_charger_unknown_semver.divert_mode()) and then place the caplog assertion (assert "Non-semver firmware version detected." in caplog.text) after the pytest.raises block; apply the same pattern to the UnknownError branch in test_set_charge_mode() so the log assertions run after the exception-raising call.openevsehttp/client.py (1)
605-618: Consider explicit async getters instead of async properties.
@property+async defyields coroutine-returning attributes. This non-idiomatic pattern can confuse callers and tooling. Since this PR is already reshaping the client surface,get_charge_current()/get_override_state()would be clearer. Tests are the only consumers found in the codebase, making a refactor straightforward.Also applies to: 971-980
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 605 - 618, Rename the coroutine-like async properties to explicit async getter methods: change async_charge_current to an async method named get_charge_current (keep the existing logic and return types) and likewise rename the other async property (the override-state getter referenced in the review) to get_override_state; update all callers (tests and any internal references) to call await get_charge_current() / await get_override_state() instead of accessing them as attributes, and remove any `@property` decorator usage on these coroutine methods so they are clearly async functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/__init__.py`:
- Around line 3-25: The package root removed several names that the
compatibility shim (__main__.py) still exposes; re-export the missing symbols by
importing ContentTypeError and ServerTimeoutError (from the exceptions module),
ERROR_TIMEOUT, INFO_LOOP_RUNNING, UPDATE_TRIGGERS (from wherever your constants
are defined), and divert_mode and states (from the module that defines them)
into openevsehttp.__init__ and add them to the __all__ list so users can still
do "from openevsehttp import ContentTypeError, ServerTimeoutError,
ERROR_TIMEOUT, INFO_LOOP_RUNNING, UPDATE_TRIGGERS, divert_mode, states" and
"import openevsehttp as oe; oe.states" without breaking compatibility.
In `@openevsehttp/client.py`:
- Around line 149-153: The websocket is created lazily in update() but
ws_start() immediately schedules calls to self.websocket.listen() and
self.websocket.keepalive(), causing a crash if update() hasn't run; modify
ws_start() (and any other code that schedules websocket tasks, e.g., the block
around where listen()/keepalive() are called) to ensure self.websocket is
initialized first by constructing an OpenEVSEWebsocket(self.url,
self._update_status, self._user, self._pwd, self._session) when self.websocket
is None before scheduling or calling self.websocket.listen() and
self.websocket.keepalive().
- Around line 407-411: The code assumes response["msg"] exists but
Requester.process_request() can return {} (or non-dict), causing KeyError and
inconsistent success handling; add a small helper method (e.g.,
_extract_msg(response) on the client) that safely handles non-dict responses,
returns the msg string or None, and optionally logs/returns a normalized value;
then replace direct response["msg"] usages in methods such as restart_wifi (the
block using response = await self.process_request(...), result =
response["msg"]) and the other occurrences (around lines referenced) to call
_extract_msg(response), compare the returned msg against allowed success values
["done","no change"], and if missing or unexpected log the full response and
raise UnknownError.
- Around line 718-721: The ambient temperature logic treats 0 as missing because
it checks temp truthily; in the method reading self._status (variables temp and
temp1) replace the truthy check with an explicit None check (e.g., if temp is
not None) so a 0°C reading is returned divided by 10, otherwise fall back to
self._status.get("temp1", 0)/10; update the branch in the function/method
handling self._status to use "is not None" for temp.
- Around line 528-531: The set_divert_mode() handler is comparing a parsed JSON
dict (returned by process_request()) to a plain string, causing the UnknownError
to be raised even on success; update set_divert_mode() to either inspect the
JSON structure (e.g., check response.get("message") or a specific status key)
for the expected success value or, if the /divertmode endpoint truly returns
plain text, call a text-returning requester method instead of process_request();
reference set_divert_mode(), process_request(), and UnknownError and ensure the
check reads the correct field from the dict (or swaps to the text path) and
include the actual response in the error log when raising UnknownError.
- Around line 671-678: The properties status and state currently index
states[...] directly which can raise on unknown or malformed state values and
status's fallback evaluates even when "status" exists; update both to perform a
safe lookup: for state() parse the raw state value from self._status (e.g.,
self._status.get("state")), coerce to int safely (with try/except or
str.isdigit), bounds-check against len(states) and return a sensible default
like "unknown" if invalid, and for status() return self._status["status"] if
present else the safe state lookup result without evaluating states[...] unless
needed; reference the status property, state property, states list, and
self._status when making these changes.
In `@openevsehttp/firmware.py`:
- Around line 123-128: The current regex "\\d\\.\\d\\.\\d" only matches
single-digit version components causing firmware_search/firwmare_filtered to be
wrong; update the search in firmware.py to match multi-digit components (e.g.
use a pattern like "\d+\.\d+\.\d+") and extract the match with
firmware_search.group(0) to produce a correct firmware_filtered before handing
it to AwesomeVersion() so feature-gate comparisons work reliably.
In `@openevsehttp/limit.py`:
- Around line 44-58: The POST payload should not be seeded from the raw GET
response: validate limit_type against valid_types first (raise InvalidType
before any network I/O), remove the await self.get() call, and construct a
minimal payload dict containing only TYPE, VALUE and optionally RELEASE, then
call self._evse.process_request(url=url, method="post", data=payload). Update
the function that currently uses await self.get(), data[TYPE], data[VALUE],
data[RELEASE] (and references to TYPE, VALUE, RELEASE, get(), process_request())
to build and send only the intended keys so 404/no-limit responses are not
reposted.
In `@openevsehttp/override.py`:
- Around line 98-100: In clear() guard against non-dict responses from
self._evse.process_request(url=..., method="delete") by checking the type and
keys before indexing response["msg"]; if response is a dict and contains "msg"
log response["msg"] via _LOGGER.debug("Toggle response: %s", ...), otherwise log
the entire response object (e.g., _LOGGER.debug("Toggle response: %s",
response)) so raw strings or empty bodies don't raise TypeError/KeyError.
- Around line 34-56: The set() method currently always overwrites
data["auto_release"] with the default True, causing unintended flips on partial
updates; change the auto_release parameter to allow None (auto_release: bool |
None = None) and only assign data["auto_release"] when auto_release is not None
(leave the value from data = await self.get() otherwise); update the type hint
and callers as needed so set() preserves existing auto_release unless an
explicit value is provided.
In `@openevsehttp/requester.py`:
- Around line 52-55: The code currently only checks for method is None before
calling getattr(session, method), causing AttributeError for invalid or
uppercase method names; update the validation around the method variable (used
with getattr(session, method)) to verify it's a string and one of the allowed
HTTP verbs (e.g., 'get','post','put','delete','patch','head','options') before
calling getattr, and if not valid raise MissingMethod so callers get the
consistent exception; implement this check in the same scope where auth and
method are handled to ensure only valid method names reach getattr(session,
method).
In `@tests/test_claims.py`:
- Around line 26-29: The logging assertions are currently placed inside the
pytest.raises context so they never run after the exception is raised; update
each test that uses pytest.raises(UnsupportedFeature) (the ones calling
test_charger_v2.list_claims, release_claim, and make_claim) to move the
caplog.at_level + caplog.text assertion outside the raises block: keep the await
call that raises inside the pytest.raises, capture logs with caplog.at_level
during that call, then after the pytest.raises block assert that "Feature not
supported for older firmware." (or the appropriate message) appears in
caplog.text.
In `@tests/test_limit.py`:
- Around line 41-44: The test currently places assertions checking caplog.text
inside the pytest.raises(UnsupportedFeature) context, making them unreachable
when the awaited call test_charger.set_limit("energy", 15) raises; move the
caplog context to wrap the raises or move the assertions outside the
pytest.raises block so that you capture logs with caplog.at_level(logging.DEBUG)
while invoking pytest.raises(UnsupportedFeature) around the await
test_charger.set_limit(...) and then assert "Feature not supported for older
firmware." in caplog.text after both contexts exit; apply the same change for
all three occurrences referencing pytest.raises, caplog.at_level,
UnsupportedFeature, and test_charger.set_limit.
In `@tests/test_override.py`:
- Around line 230-243: The assertions placed after awaited calls inside
pytest.raises are unreachable; update test_set_override to split each failing
call into its own pytest.raises context and move related caplog assertions
outside that context so they run after the exception is caught: wrap await
test_charger.set_override("invalid") with with pytest.raises(ValueError) inside
caplog.at_level and then assert "Invalid override state: invalid" in
caplog.text; do the same for await test_charger_v2.set_override("active") and
await test_charger_unknown_semver.set_override("active") using with
pytest.raises(UnsupportedFeature) and then check that "Feature not supported for
older firmware." appears in caplog.text; keep using
caplog.at_level(logging.DEBUG) around each pair so the log capture is active.
In `@tests/test_sensors.py`:
- Around line 30-33: The tests currently place the caplog assertions inside the
pytest.raises context so they never run after UnsupportedFeature is raised;
update each test that calls test_charger_v2.grid_voltage,
test_charger_v2.self_production, and test_charger_v2.soc to move the
caplog.at_level/assert "Feature not supported for older firmware." checks to
immediately after the corresponding with pytest.raises(UnsupportedFeature) block
so the exception is still asserted but the captured log text is inspected
afterwards (use the same caplog fixture and logging.DEBUG level).
---
Nitpick comments:
In `@openevsehttp/client.py`:
- Around line 605-618: Rename the coroutine-like async properties to explicit
async getter methods: change async_charge_current to an async method named
get_charge_current (keep the existing logic and return types) and likewise
rename the other async property (the override-state getter referenced in the
review) to get_override_state; update all callers (tests and any internal
references) to call await get_charge_current() / await get_override_state()
instead of accessing them as attributes, and remove any `@property` decorator
usage on these coroutine methods so they are clearly async functions.
In `@tests/test_client.py`:
- Around line 783-786: The test currently wraps both the awaited call and the
caplog assertion inside pytest.raises, so the assertion on caplog.text never
executes; change both occurrences to only await the raising call inside the
pytest.raises context (e.g., await test_charger_unknown_semver.divert_mode())
and then place the caplog assertion (assert "Non-semver firmware version
detected." in caplog.text) after the pytest.raises block; apply the same pattern
to the UnknownError branch in test_set_charge_mode() so the log assertions run
after the exception-raising call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc3c1bd5-843a-42d9-8174-e777480bfe4d
📒 Files selected for processing (19)
openevsehttp/__init__.pyopenevsehttp/__main__.pyopenevsehttp/claims.pyopenevsehttp/client.pyopenevsehttp/firmware.pyopenevsehttp/limit.pyopenevsehttp/override.pyopenevsehttp/requester.pyopenevsehttp/sensors.pytests/const.pytests/test_claims.pytests/test_client.pytests/test_firmware.pytests/test_limit.pytests/test_main.pytests/test_main_edge_cases.pytests/test_override.pytests/test_requester.pytests/test_sensors.py
💤 Files with no reviewable changes (1)
- tests/test_main_edge_cases.py
… for firmware, sensors, and websocket functionality
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_websocket.py (1)
166-168:⚠️ Potential issue | 🟡 Minor
ws_client_authbuilds a malformed base URL.
SERVER_URLalready includeshttp://, so Line 167 becomeshttp://http://openevse.test.tld/. The patchedws_connecthides it today, but any assertion on the real URI will be exercising the wrong input.Possible fix
client = OpenEVSEWebsocket( - f"http://{SERVER_URL}", callback, user="test", password="pw" + SERVER_URL, callback, user="test", password="pw" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_websocket.py` around lines 166 - 168, The test creates a malformed base URL by prefixing "http://" to SERVER_URL (which already contains a scheme); update the test instantiation of OpenEVSEWebsocket in ws_client_auth so it uses SERVER_URL as-is (or strip any existing scheme before prefixing) instead of f"http://{SERVER_URL}", ensuring the OpenEVSEWebsocket constructor receives a properly formed URL for assertions and ws_connect checks.
♻️ Duplicate comments (5)
openevsehttp/client.py (5)
167-190:⚠️ Potential issue | 🔴 CriticalInitialize the websocket before scheduling listener tasks.
update()is still the only place that constructsself.websocket, butws_start()falls straight through to_start_listening()on a fresh client. Line 188 then dereferencesNone.listen(), soOpenEVSE(...).ws_start()still crashes unless callers remembered to callupdate()first.Possible fix
def ws_start(self) -> None: """Start the websocket listener.""" + if self.websocket is None: + self.websocket = OpenEVSEWebsocket( + self.url, self._update_status, self._user, self._pwd, self._session + ) if self.websocket: if self._ws_listening and self.websocket.state == "connected": raise AlreadyListening if self._ws_listening and self.websocket.state != "connected": self._ws_listening = False self._start_listening()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 167 - 190, ws_start() can call _start_listening() before self.websocket is created (it's constructed in update()), causing a None.listen() crash; update ws_start()/_start_listening() to ensure self.websocket is initialized before scheduling tasks by checking if self.websocket is None and either creating/setting it (or raising a clear error) before calling self.websocket.listen() and self.websocket.keepalive, e.g., guard before create_task(self.websocket.listen()) and create_task(self.repeat(...)) using the websocket attribute (symbols: ws_start, _start_listening, self.websocket, update, listen, keepalive, repeat, _ws_listening, _loop).
662-669:⚠️ Potential issue | 🟠 MajorUse safe state lookups for unknown or malformed codes.
dict.get()evaluates its default eagerly, so Line 664 can still raise on an unexpectedstateeven when_status["status"]exists. Line 669 has the same direct-indexing problem.Possible fix
`@property` def status(self) -> str: """Return charger's state.""" - return self._status.get("status", states[int(self._status.get("state", 0))]) + if (status := self._status.get("status")) is not None: + return status + try: + return states.get(int(self._status.get("state", 0)), "unknown") + except (TypeError, ValueError): + return "unknown" `@property` def state(self) -> str: """Return charger's state.""" - return states[int(self._status.get("state", 0))] + try: + return states.get(int(self._status.get("state", 0)), "unknown") + except (TypeError, ValueError): + return "unknown"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 662 - 669, The status() method and state property perform unsafe lookups that can raise if _status["state"] is missing or malformed because dict.get evaluates its default eagerly and direct indexing into states may fail; update both to first retrieve the raw_state = self._status.get("state") (or self._status.get("status") check for status()), then safely parse it to an int inside a try/except or use a bounded fallback index, and only fall back to self._status.get("status") (for status()) or a safe default string (for state) when parsing fails; reference the status() function, state property, the _status dict, and the states list when making this change.
398-402:⚠️ Potential issue | 🟠 MajorNormalize the command response before reading
msg.These call sites still assume
process_request()always returns a dict with amsgkey. On the non-JSON/error path that turns bad responses intoKeyErrors, andrestart_wifi()/restart_evse()still accept anymsgvalue as success.Safer pattern
- result = response["msg"] + result = response.get("msg") if isinstance(response, dict) else None if result not in ["done", "no change"]: - _LOGGER.error("Problem issuing command: %s", response["msg"]) + _LOGGER.error("Problem issuing command: %s", response) raise UnknownErrorAlso applies to: 463-468, 475-476, 484-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 398 - 402, The call assumes process_request() returns a dict with a "msg" key; change the callers (the block using response = await self.process_request(...), and similar call sites in restart_wifi() and restart_evse()) to first normalize and validate the response: ensure response is a dict, extract msg = response.get("msg") (or set msg = None if not present), and only treat success when msg in ("done", "no change"); on non-dict or missing msg log the entire response object and raise UnknownError (include response content in the log message for debugging). Also apply the same normalization pattern to the other affected blocks around the references at the noted locations so KeyError is avoided and unexpected responses are handled consistently.
507-522:⚠️ Potential issue | 🔴 Critical
set_divert_mode()still assumes a raw text return.This branch compares the whole
process_request()result to"Divert Mode changed". On the normalized dict/empty-dict path, that check never matches, so the new success case intests/test_client.pyat Lines 1440-1443 will still raiseUnknownError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 507 - 522, The check in set_divert_mode uses the raw-text compare against "Divert Mode changed" so it fails when process_request returns the normalized dict/empty dict; update set_divert_mode to treat the normalized success responses as success by inspecting the response type and/or content from process_request (accept an empty dict or a dict indicating success) in addition to the legacy string; modify the success check in set_divert_mode (which uses process_request, divert_mode_map, and raises UnknownError) to return/continue on either the legacy "Divert Mode changed" string or an empty/normalized dict indicating success, otherwise log and raise UnknownError as before.
707-712:⚠️ Potential issue | 🟡 MinorDon't treat
0°C as a missing ambient reading.Line 710 uses truthiness, so a valid zero reading falls through to
temp1instead of returning0.0.Possible fix
temp = self._status.get("temp") - if temp: + if temp is not None and not isinstance(temp, bool): return temp / 10 return self._status.get("temp1", 0) / 10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 707 - 712, The function ambient_temperature incorrectly treats a 0 reading as missing because it checks temp with truthiness; change the check in ambient_temperature to test for None explicitly (e.g. use "if temp is not None") so that a legitimate 0 value from self._status["temp"] returns 0.0; keep the fallback behavior using self._status.get("temp1", 0) / 10 unchanged (or explicitly check temp1 for None if you prefer) and reference the ambient_temperature method and the self._status keys "temp" and "temp1" when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_client.py`:
- Around line 783-786: The assertion checking caplog must be moved out of the
context manager that expects the exception: change the pattern around the call
to test_charger_unknown_semver.divert_mode() so that you use "with
pytest.raises(UnsupportedFeature): await
test_charger_unknown_semver.divert_mode()" and then, after that with-block, use
"with caplog.at_level(logging.DEBUG):" (or simply inspect caplog.text) and
assert that "Non-semver firmware version detected." is present; apply the same
fix to the other similar test blocks (the other occurrence(s) that currently
nest caplog assertions inside the pytest.raises block).
- Around line 1387-1393: The test function test_restart_wifi is defined twice
which causes a name collision (Ruff F811); locate the duplicate
test_restart_wifi definitions and either remove the redundant one or give it a
unique name (or merge their assertions) so each test function name is unique;
update any related fixtures/uses accordingly and run tests to ensure only one
test_restart_wifi remains.
- Around line 814-816: The test assertion expects "Restart response: restart
gateway" but openevsehttp/client.py's restart_wifi() logs "WiFi Restart
response: %s"; update the test to assert the actual log text produced by
restart_wifi() (e.g., look for "WiFi Restart response: restart gateway") or
change the restart_wifi() logging call to match the test expectation; locate the
test call to test_charger_modified_ver.restart_wifi() and the logging in
restart_wifi() to apply the consistent string.
- Around line 1407-1412: The DEBUG caplog context does not cover the RAPI
restart branch, so move or extend the caplog.at_level(logging.DEBUG) context to
include the block that sets charger._config["version"]="4.0.0" and the with
patch.object(...) that calls charger.restart_evse(); ensure the assert
"Restarting EVSE module via RAPI" in caplog.text runs while caplog is at DEBUG
level (i.e., wrap the version change, the patch of charger.send_command
(AsyncMock) and the await charger.restart_evse() call inside the same
caplog.at_level(...) context) so logging state doesn't leak from earlier parts
of the test.
In `@tests/test_firmware.py`:
- Around line 1-317: This file fails ruff format checks; run the formatter (ruff
format) on the test file containing functions like test_firmware_check,
test_version_check, test_firmware_check_no_config, etc., to fix
whitespace/formatting issues, then stage and commit the formatted file so CI's
ruff format --check passes; alternatively run your project's pre-commit hooks or
black/ruff integration to apply the same formatting automatically.
---
Outside diff comments:
In `@tests/test_websocket.py`:
- Around line 166-168: The test creates a malformed base URL by prefixing
"http://" to SERVER_URL (which already contains a scheme); update the test
instantiation of OpenEVSEWebsocket in ws_client_auth so it uses SERVER_URL as-is
(or strip any existing scheme before prefixing) instead of
f"http://{SERVER_URL}", ensuring the OpenEVSEWebsocket constructor receives a
properly formed URL for assertions and ws_connect checks.
---
Duplicate comments:
In `@openevsehttp/client.py`:
- Around line 167-190: ws_start() can call _start_listening() before
self.websocket is created (it's constructed in update()), causing a
None.listen() crash; update ws_start()/_start_listening() to ensure
self.websocket is initialized before scheduling tasks by checking if
self.websocket is None and either creating/setting it (or raising a clear error)
before calling self.websocket.listen() and self.websocket.keepalive, e.g., guard
before create_task(self.websocket.listen()) and create_task(self.repeat(...))
using the websocket attribute (symbols: ws_start, _start_listening,
self.websocket, update, listen, keepalive, repeat, _ws_listening, _loop).
- Around line 662-669: The status() method and state property perform unsafe
lookups that can raise if _status["state"] is missing or malformed because
dict.get evaluates its default eagerly and direct indexing into states may fail;
update both to first retrieve the raw_state = self._status.get("state") (or
self._status.get("status") check for status()), then safely parse it to an int
inside a try/except or use a bounded fallback index, and only fall back to
self._status.get("status") (for status()) or a safe default string (for state)
when parsing fails; reference the status() function, state property, the _status
dict, and the states list when making this change.
- Around line 398-402: The call assumes process_request() returns a dict with a
"msg" key; change the callers (the block using response = await
self.process_request(...), and similar call sites in restart_wifi() and
restart_evse()) to first normalize and validate the response: ensure response is
a dict, extract msg = response.get("msg") (or set msg = None if not present),
and only treat success when msg in ("done", "no change"); on non-dict or missing
msg log the entire response object and raise UnknownError (include response
content in the log message for debugging). Also apply the same normalization
pattern to the other affected blocks around the references at the noted
locations so KeyError is avoided and unexpected responses are handled
consistently.
- Around line 507-522: The check in set_divert_mode uses the raw-text compare
against "Divert Mode changed" so it fails when process_request returns the
normalized dict/empty dict; update set_divert_mode to treat the normalized
success responses as success by inspecting the response type and/or content from
process_request (accept an empty dict or a dict indicating success) in addition
to the legacy string; modify the success check in set_divert_mode (which uses
process_request, divert_mode_map, and raises UnknownError) to return/continue on
either the legacy "Divert Mode changed" string or an empty/normalized dict
indicating success, otherwise log and raise UnknownError as before.
- Around line 707-712: The function ambient_temperature incorrectly treats a 0
reading as missing because it checks temp with truthiness; change the check in
ambient_temperature to test for None explicitly (e.g. use "if temp is not None")
so that a legitimate 0 value from self._status["temp"] returns 0.0; keep the
fallback behavior using self._status.get("temp1", 0) / 10 unchanged (or
explicitly check temp1 for None if you prefer) and reference the
ambient_temperature method and the self._status keys "temp" and "temp1" when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff719302-f0be-4c8b-a9a9-0ad13080304d
📒 Files selected for processing (6)
openevsehttp/client.pytests/test_client.pytests/test_firmware.pytests/test_requester.pytests/test_sensors.pytests/test_websocket.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_sensors.py
- tests/test_requester.py
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
openevsehttp/client.py (1)
695-697:⚠️ Potential issue | 🟡 MinorAvoid eagerly computing the fallback in
status.Line 697 still evaluates
self.stateeven when"status"is present, becausedict.get()evaluates its default argument eagerly. That keeps the fallback path active on ordinarystatusreads.Possible fix
`@property` def status(self) -> str: """Return charger's state.""" - return self._status.get("status", self.state) + status = self._status.get("status") + if status is not None: + return status + return self.state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 695 - 697, The status() method eagerly evaluates self.state because dict.get evaluates its default argument; change the implementation in status to avoid calling self.state when a "status" key exists by checking membership first (e.g., if "status" in self._status: return self._status["status"] else return self.state), referencing the status() function and the self._status and self.state symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 202-207: The reconnect path overwrites self.tasks even when prior
websocket tasks may still be running, causing duplicate listeners/keepalives;
change the ws_start()/setup block that checks self._ws_listening to first
inspect self.tasks and only create new tasks if there are no active (not
done/cancelled) tasks — e.g. filter existing self.tasks for .done()/.cancelled()
and reuse or extend that list instead of assigning a new list, or skip creating
new websocket.listen()/repeat() tasks when an active task is already present for
those coroutines; apply the same fix to the other identical block referenced
(the 216-223 area) and ensure self._ws_listening is set appropriately after
tasks are created.
- Around line 250-260: The ws_disconnect method must be idempotent: remove the
assert and early-return if there is nothing to do by checking self.websocket and
self.tasks; specifically, in ws_disconnect() check if self._ws_listening is
False and both self.websocket is None and self.tasks is falsy then simply
return, otherwise cancel and await any existing self.tasks (if not None) and
only call await self.websocket.close() when self.websocket is not None, then
clear self.tasks and/or set self.websocket to None so repeated calls are safe;
reference ws_disconnect, self.tasks, self.websocket and self._ws_listening when
making the changes.
- Around line 533-538: The LED brightness setter (the block that sets
data["led_brightness"]=level and calls await self.process_request(url=...,
method="post", data=data)) currently treats any non-exception response as
success; update this to validate the response payload from
self.process_request() and raise UnknownError when the response is empty,
missing expected keys, or indicates failure (mirror validation used by other
config mutation methods). Specifically, capture the return value of
process_request into a variable, check for the expected success marker/fields
(as other config methods do), and raise UnknownError with a clear message if the
payload is unexpected or indicates rejection.
In `@openevsehttp/override.py`:
- Around line 88-90: The current override toggle discards the boolean result
from self._evse.send_command and treats every response as success; change the
code in override.py where command = "$FE" ... _, msg = await
self._evse.send_command(command) to capture the first element (e.g., success,
msg = await self._evse.send_command(command)), check success and handle failure:
if success is False, log an error with the msg (use _LOGGER.error) and
return/raise so the no-op on 3.x firmware is not treated as a successful toggle;
otherwise proceed to _LOGGER.debug on success. Ensure you reference send_command
and self._evse so the fix locates the correct call site.
In `@openevsehttp/requester.py`:
- Line 49: process_request() currently sometimes returns raw strings for
non-JSON responses which breaks callers like send_command() and Override.set()
that expect a dict; update process_request (and the non-JSON branches around the
101-127 area) to always return a predictable dict shape (e.g. {"status_code":
response.status_code, "body": response.json() if JSON else response.text, "ok":
response.ok}) or raise a clear exception for non-2xx responses; modify the
non-JSON-returning branches to construct and return that dict (or raise), so
callers such as send_command() and Override.set() receive a consistent mapping
instead of raw str.
In `@openevsehttp/websocket.py`:
- Around line 61-67: The fallback currently uses asyncio.get_event_loop() which
can raise RuntimeError from worker threads and also doesn't target the correct
loop; modify the websocket class to capture and store the running event loop
(e.g., self._loop) during initialization (or when connection is first
established) and replace the try/except with a safe scheduling strategy: attempt
asyncio.create_task(coro) on the current loop, and on RuntimeError use
asyncio.run_coroutine_threadsafe(coro, self._loop) (or
self._loop.call_soon_threadsafe to create_task on that loop), and retain the
returned Task/Future (log or attach a done callback to surface exceptions).
Update references around self.callback and SIGNAL_CONNECTION_STATE so the
callback is scheduled on the stored loop and its errors are not silently
discarded.
---
Duplicate comments:
In `@openevsehttp/client.py`:
- Around line 695-697: The status() method eagerly evaluates self.state because
dict.get evaluates its default argument; change the implementation in status to
avoid calling self.state when a "status" key exists by checking membership first
(e.g., if "status" in self._status: return self._status["status"] else return
self.state), referencing the status() function and the self._status and
self.state symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 813eef80-844a-46f4-815a-62d7c7fe9df2
📒 Files selected for processing (18)
openevsehttp/__init__.pyopenevsehttp/__main__.pyopenevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/firmware.pyopenevsehttp/limit.pyopenevsehttp/override.pyopenevsehttp/requester.pyopenevsehttp/websocket.pytests/test_claims.pytests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_limit.pytests/test_override.pytests/test_requester.pytests/test_sensors.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (4)
- openevsehttp/const.py
- tests/test_firmware.py
- tests/test_claims.py
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_websocket.py
- tests/test_limit.py
- tests/test_sensors.py
- tests/test_override.py
- tests/test_requester.py
…bsocket connection handling and error reporting
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openevsehttp/override.py (1)
104-105:⚠️ Potential issue | 🟡 MinorMinor: Log message says "Toggle response" in
clear()method.This appears to be a copy-paste artifact. Consider changing to "Clear response" for accuracy.
🔧 Suggested fix
msg = response.get("msg") if isinstance(response, dict) else response - _LOGGER.debug("Toggle response: %s", msg) + _LOGGER.debug("Clear response: %s", msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/override.py` around lines 104 - 105, Log in the clear() method incorrectly says "Toggle response"; update the debug message to accurately reflect the method by changing the _LOGGER.debug call in clear() (the line that builds msg from response and logs it) to use "Clear response: %s" (keep using msg variable) so the log matches the clear() action.
🧹 Nitpick comments (2)
openevsehttp/requester.py (2)
39-41: Consider adding type hints for the callback parameter.The
set_update_callbackmethod accepts a callback but lacks type annotations. Based on line 127 where it's awaited, this should be typed asCallable[[], Awaitable[None]] | None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 39 - 41, Add a type annotation to the set_update_callback parameter and the stored attribute to reflect that the callback is asynchronous: update the signature of set_update_callback(callback) to accept Callable[[], Awaitable[None]] | None and update the type of self._update_callback accordingly (referencing set_update_callback and self._update_callback where the callback is awaited later).
118-119: Consider whether 404/405/500 should raise or return a distinguishable response.These status codes only log a warning but don't raise exceptions or return an error indicator. Callers may not realize the request failed unless they inspect the response content. Consider adding a status indicator to the returned dict (e.g.,
{"msg": ..., "ok": False}) or documenting the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 118 - 119, The code path that checks resp.status and currently only calls _LOGGER.warning("%s", message) should either raise or return an explicit failure indicator so callers can detect errors; update the handler that inspects resp.status (the block referencing resp.status, _LOGGER.warning and message) to return a consistent dict such as {"msg": message, "ok": False, "status": resp.status} (or raise a specific exception) for statuses [404, 405, 500], and ensure all call sites expect and propagate that shape (or catch the exception) instead of silently continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_client.py`:
- Around line 1570-1579: The test starts the websocket twice but never asserts
the expected exception; update the test to assert that calling
charger.ws_start() a second time raises the AlreadyListening exception (use
pytest.raises(AlreadyListening) around the second ws_start call) and optionally
keep the caplog checks (e.g., assert the "Websocket not initialized,
creating..." message appears only once while "Setting up websocket ping..." may
appear on re-setup) to validate logging behavior; reference the ws_start method
and the AlreadyListening exception in your assertion so the test fails if the
second start does not raise.
---
Duplicate comments:
In `@openevsehttp/override.py`:
- Around line 104-105: Log in the clear() method incorrectly says "Toggle
response"; update the debug message to accurately reflect the method by changing
the _LOGGER.debug call in clear() (the line that builds msg from response and
logs it) to use "Clear response: %s" (keep using msg variable) so the log
matches the clear() action.
---
Nitpick comments:
In `@openevsehttp/requester.py`:
- Around line 39-41: Add a type annotation to the set_update_callback parameter
and the stored attribute to reflect that the callback is asynchronous: update
the signature of set_update_callback(callback) to accept Callable[[],
Awaitable[None]] | None and update the type of self._update_callback accordingly
(referencing set_update_callback and self._update_callback where the callback is
awaited later).
- Around line 118-119: The code path that checks resp.status and currently only
calls _LOGGER.warning("%s", message) should either raise or return an explicit
failure indicator so callers can detect errors; update the handler that inspects
resp.status (the block referencing resp.status, _LOGGER.warning and message) to
return a consistent dict such as {"msg": message, "ok": False, "status":
resp.status} (or raise a specific exception) for statuses [404, 405, 500], and
ensure all call sites expect and propagate that shape (or catch the exception)
instead of silently continuing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33e7deeb-2b61-42ba-b0d6-97067187557f
📒 Files selected for processing (6)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_override.pytests/test_requester.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_override.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_requester.py
- openevsehttp/client.py
…ate logging labels
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
openevsehttp/client.py (1)
202-213:⚠️ Potential issue | 🟠 MajorReconnect can still orphan live websocket tasks.
_update_status()flips_ws_listeningtoFalseon disconnect before the previouslisten()/repeat()tasks are guaranteed to finish. Because this condition usesor, a reconnect whileactive_tasksis non-empty still replacesself.tasks, recreating the duplicate-listener/leaked-handle problem.Suggested fix
active_tasks = [] if self.tasks: active_tasks = [t for t in self.tasks if not t.done()] - if not self._ws_listening or not active_tasks: - _LOGGER.debug("Setting up websocket ping...") - self.tasks = [ - self._loop.create_task(self.websocket.listen()), - self._loop.create_task(self.repeat(300, self.websocket.keepalive)), - ] - self._ws_listening = True + if active_tasks: + _LOGGER.debug("Existing websocket tasks still active, skipping restart") + return + + _LOGGER.debug("Setting up websocket ping...") + self.tasks = [ + self._loop.create_task(self.websocket.listen()), + self._loop.create_task(self.repeat(300, self.websocket.keepalive)), + ] + self._ws_listening = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 202 - 213, The reconnect logic can replace live websocket tasks because _update_status() sets self._ws_listening = False before previous listen()/repeat() tasks finish; change the condition that creates new tasks in the block using self.tasks/self._ws_listening from "if not self._ws_listening or not active_tasks:" to require both (e.g., "if not self._ws_listening and not active_tasks:") and before assigning self.tasks ensure any remaining active_tasks are cleanly cancelled/awaited (iterate active_tasks, call cancel(), then await them or gather with return_exceptions) so websocket.listen() and repeat() aren't orphaned; update references to websocket.listen(), repeat(), self.tasks, and self._ws_listening accordingly.openevsehttp/override.py (1)
87-93:⚠️ Potential issue | 🟠 MajorHarden the 3.x toggle path before sending RAPI.
_statusstarts empty untilupdate()runs, so the direct index on Line 88 can raise on a fresh client. Also,send_command()returns(cmd, ret), so a$NK^..reply still makessuccess == "OK"and this branch logs the failed toggle as success.Suggested fix
# Older firmware use RAPI commands _LOGGER.debug("Toggling manual override via RAPI") - command = "$FE" if self._evse._status["state"] == 254 else "$FS" - success, msg = await self._evse.send_command(command) - if not success: + state = self._evse._status.get("state") + if state is None: + await self._evse.update() + state = self._evse._status.get("state") + if state is None: + _LOGGER.error("Unable to determine override state before toggle") + raise UnknownError + command = "$FE" if state == 254 else "$FS" + _, msg = await self._evse.send_command(command) + if not str(msg).startswith("$OK"): _LOGGER.error("Problem issuing command. Response: %s", msg) raise UnknownError _LOGGER.debug("Toggle response: %s", msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/override.py` around lines 87 - 93, The toggle path should guard against an empty _status and correctly interpret send_command()'s return: first ensure we have a current state by calling or awaiting self._evse.update() if _status is empty (or use self._evse._status.get("state", 0) as a safe default) before computing command, and then unpack the send_command() result into (cmd, resp) rather than (success, msg); check the response string (e.g., resp startswith "OK" or parse the RAPI reply) to determine success, log the actual resp on failure, and raise UnknownError when the response indicates failure.tests/test_client.py (1)
1396-1399:⚠️ Potential issue | 🟡 MinorMove the
caplogchecks out of thepytest.raises()blocks.The awaited call exits the block as soon as
UnknownErroris raised, so these log assertions never execute. That leaves all of these failure-log branches effectively untested.Also applies to: 1554-1568, 1593-1622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 1396 - 1399, The test currently places caplog assertions inside the pytest.raises(UnknownError) context (around the await charger.set_service_level(1)), so the assertions never run because the context exits when the exception is raised; move the caplog inspection out of the pytest.raises block: keep the with pytest.raises(UnknownError): await charger.set_service_level(1) to assert the exception, then immediately after that block (still inside with caplog.at_level(logging.ERROR):) assert that the expected log message (e.g., "Problem issuing command. Response: {'msg': 'failed'}") appears in caplog.text; apply the same change to the other occurrences referenced (around lines 1554-1568 and 1593-1622) so all log assertions execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 217-235: The connection-state callback dereferences
self.websocket.uri after ws_disconnect() can set self.websocket = None; update
the handler (the branch handling msgtype == SIGNAL_CONNECTION_STATE in the
methods that set _ws_listening and log websocket.uri) to first guard access to
self.websocket by either returning early if self.websocket is None or capturing
uri = getattr(self.websocket, "uri", None) and using that in _LOGGER.debug
calls, and ensure the same guard is applied to the other similar block around
the STATE_STOPPED/STATE_DISCONNECTED handling (also referenced at the later
block around lines 268-274) so late callbacks during shutdown do not raise when
self.websocket is cleared.
- Around line 483-489: The set_current() flow in set_current (uses
self._version_check and await self.send_command) only logs the RAPI reply so
replies like "$NK^.." or (False, ...) from 2.x/3.x firmware are treated as
successes; update set_current to inspect the reply returned by send_command
(both the boolean status and the message string), detect rejection patterns
(e.g. response starting with "$NK" or send_command returning False), and
return/raise an error or False and log an error when the charger rejected the
limit instead of treating any reply as success; use the existing symbols
set_current, send_command, and _version_check to locate and change the branch
where command = f"$SC {amps} N" / f"$SC {amps} V" and add explicit validation of
msg and the boolean result.
In `@openevsehttp/override.py`:
- Around line 69-72: The override write methods (set(), toggle() path, clear())
currently return the raw response from Requester.process_request() which may be
{"ok": False, "status": ...} for HTTP errors; update each method (the
POST/DELETE calls that call self._evse.process_request()) to inspect the
returned response and raise an exception when response.get("ok") is falsy —
include the status and any error/body in the exception message so callers (e.g.,
OpenEVSE.set_current()) surface failures instead of silently no‑opping; do this
for the POST path shown (current code using _evse.process_request(url=url,
method="post", data=data)) and the analogous locations used by toggle() and
clear().
In `@openevsehttp/requester.py`:
- Around line 110-127: The current branch only handles 400/401/404/405/500 and
lets other error statuses fall through; update the error handling in
requester.py so that after the special-case blocks for resp.status == 400
(raising ParseJSONError) and resp.status == 401 (raising AuthenticationError),
any resp.status >= 400 is treated consistently: log the response (use
_LOGGER.warning or _LOGGER.error as appropriate), ensure message is a dict and
add/override {"ok": False, "status": resp.status} (if message is not a dict,
convert it to {"msg": message, "ok": False, "status": resp.status}), and then
return the normalized message instead of allowing success-path processing; keep
the existing behavior for ParseJSONError and AuthenticationError and reuse the
existing symbols resp.status, message, ParseJSONError, AuthenticationError, and
_LOGGER to locate the code to change.
In `@tests/test_requester.py`:
- Around line 32-34: The assertions placed inside the pytest.raises() context
are dead because the awaited call (test_charger_auth_err.update()) raises and
control exits the block; move the post-exception assertions (e.g., assert
test_charger_auth_err is None) to immediately after the with
pytest.raises(main.AuthenticationError): block so they execute, and repeat the
same fix for the other occurrences mentioned (lines 90-109 and 136-138) that use
pytest.raises with trailing asserts inside the block. Ensure each with
pytest.raises(...) only wraps the call that should raise and any state/variable
assertions are performed after the block.
---
Duplicate comments:
In `@openevsehttp/client.py`:
- Around line 202-213: The reconnect logic can replace live websocket tasks
because _update_status() sets self._ws_listening = False before previous
listen()/repeat() tasks finish; change the condition that creates new tasks in
the block using self.tasks/self._ws_listening from "if not self._ws_listening or
not active_tasks:" to require both (e.g., "if not self._ws_listening and not
active_tasks:") and before assigning self.tasks ensure any remaining
active_tasks are cleanly cancelled/awaited (iterate active_tasks, call cancel(),
then await them or gather with return_exceptions) so websocket.listen() and
repeat() aren't orphaned; update references to websocket.listen(), repeat(),
self.tasks, and self._ws_listening accordingly.
In `@openevsehttp/override.py`:
- Around line 87-93: The toggle path should guard against an empty _status and
correctly interpret send_command()'s return: first ensure we have a current
state by calling or awaiting self._evse.update() if _status is empty (or use
self._evse._status.get("state", 0) as a safe default) before computing command,
and then unpack the send_command() result into (cmd, resp) rather than (success,
msg); check the response string (e.g., resp startswith "OK" or parse the RAPI
reply) to determine success, log the actual resp on failure, and raise
UnknownError when the response indicates failure.
In `@tests/test_client.py`:
- Around line 1396-1399: The test currently places caplog assertions inside the
pytest.raises(UnknownError) context (around the await
charger.set_service_level(1)), so the assertions never run because the context
exits when the exception is raised; move the caplog inspection out of the
pytest.raises block: keep the with pytest.raises(UnknownError): await
charger.set_service_level(1) to assert the exception, then immediately after
that block (still inside with caplog.at_level(logging.ERROR):) assert that the
expected log message (e.g., "Problem issuing command. Response: {'msg':
'failed'}") appears in caplog.text; apply the same change to the other
occurrences referenced (around lines 1554-1568 and 1593-1622) so all log
assertions execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f5b21e7-c5a8-45d1-96e7-ff7f64e04be9
📒 Files selected for processing (7)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_limit.pytests/test_override.pytests/test_requester.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_limit.py
- tests/test_override.py
…nd add test coverage for websocket and RAPI error states.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
openevsehttp/client.py (1)
123-123: Minor typo in comment.
addiontalshould beadditional.✏️ Fix typo
- # TODO: add addiontal endpoints to update + # TODO: add additional endpoints to update🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` at line 123, Fix the typo in the TODO comment that currently reads "add addiontal endpoints to update" by changing "addiontal" to "additional" so the comment reads "TODO: add additional endpoints to update"; locate the TODO comment string in openevsehttp/client.py and update only the comment text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 294-310: The ws_state property currently asserts self.websocket
which raises AssertionError when websocket is None and crashes background tasks;
change ws_state to safely handle a missing websocket by returning a default
state (e.g. "stopped") when self.websocket is None instead of asserting, e.g.
remove the assert and return self.websocket.state if present else "stopped"
(adjust the return type to Optional/str if needed), leaving repeat() unchanged
so its while self.ws_state != "stopped" check no longer crashes when
ws_disconnect() sets self.websocket = None.
- Around line 462-469: The assert self._config line in divert_mode() is unsafe
because self._config is initialized to {} and an empty dict is falsy; remove
that assertion and rely on the existing key check for "divert_enabled" on
self._config (or replace the assert with an explicit None-check like if
self._config is None: raise UnsupportedFeature) so calling divert_mode() before
update() won't raise an AssertionError; reference symbols: divert_mode(),
self._config, and the "divert_enabled" key.
In `@tests/test_requester.py`:
- Around line 222-236: The test test_process_request_unicode_decode_error
currently patches aiohttp.ClientResponse.text which doesn't affect aioresponses
mocks; instead update the mock_aioclient.get call used by the test to trigger a
UnicodeDecodeError via aioresponses' exception mechanism so
OpenEVSE.process_request actually encounters the decode error and follows the
resp.read() fallback path (i.e. set the GET mock to raise UnicodeDecodeError
rather than patching ClientResponse.text).
---
Nitpick comments:
In `@openevsehttp/client.py`:
- Line 123: Fix the typo in the TODO comment that currently reads "add addiontal
endpoints to update" by changing "addiontal" to "additional" so the comment
reads "TODO: add additional endpoints to update"; locate the TODO comment string
in openevsehttp/client.py and update only the comment text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2717812-67f3-450a-8eba-bcd0d58fa09f
📒 Files selected for processing (7)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_coverage_gap.pytests/test_override.pytests/test_requester.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_override.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openevsehttp/requester.py
- openevsehttp/override.py
…code error test case Signed-off-by: firstof9@gmail.com <firstof9@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
openevsehttp/client.py (1)
505-509:⚠️ Potential issue | 🟠 Major
send_command()does not return a boolean success flag.
Requester.send_command()returns(cmd, ret/msg), sosuccesshere is usually"OK"or the echoed command string.not successtherefore never trips on real legacy replies, and this path only detects failures when the second tuple item happens to start with$NK.🧭 Suggested fix
- success, msg = await self.send_command(command) - if not success or (isinstance(msg, str) and msg.startswith("$NK")): + _, msg = await self.send_command(command) + if not isinstance(msg, str) or not msg.startswith("$OK"): _LOGGER.error("Problem setting current limit. Response: %s", msg) return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 505 - 509, The code assumes send_command returns a boolean "success" but Requester.send_command() actually returns (cmd, ret/msg); change the unpacking to "cmd, msg = await self.send_command(command)" and update the error check to inspect the actual returned strings (e.g. treat an error if either cmd or msg is a string starting with "$NK" or msg is None/empty) instead of using "if not success"; update the log line to use the new variables (cmd, msg) and return False on that condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 467-469: The toggle logic in divert_mode() reads next state from
self._config["divert_enabled"] but neither checks the POST response nor updates
the cached flag, so failures are treated as successes and subsequent calls use
stale state; modify divert_mode() to inspect the server response (reject when
payload contains {"ok": False} or other error indicators), only set
self._config["divert_enabled"] to the new value after a successful POST, and
raise or return an error on failure; apply the same validation/update pattern to
the other branch handling (lines ~477-480) so both code paths consistently
validate the response and update the cached flag.
- Around line 263-267: Wrap the callback invocation in a try/except so
exceptions raised by user handlers do not escape the websocket receive loop: in
the block that currently checks is_coroutine_function(self.callback) and then
awaits or calls self.callback, surround the await/self.callback() with
try/except Exception as e and log the error (e.g. self.logger.exception(...) or
logging.exception(...)) instead of re-raising; do this for both the coroutine
and non-coroutine branches so any exception from self.callback is caught and
swallowed (after logging) and will not stop _update_status()/the websocket task.
In `@tests/test_requester.py`:
- Around line 534-535: The assertion `assert charger._status is not None` is
vacuous because OpenEVSE.__init__ sets `_status` to an empty dict; instead
verify that the callback actually ran by asserting a concrete field populated by
update() (e.g., `config_version`) exists and/or matches the returned `result`
(for example assert `"config_version" in charger._status and
charger._status["config_version"] == result["config_version"]`), or
alternatively replace the vacuous assertion with a spy/assert that
`charger.update()` (or the specific callback method) was called; refer to
OpenEVSE.__init__, charger._status, update(), and the `config_version` callback
when making the change.
---
Duplicate comments:
In `@openevsehttp/client.py`:
- Around line 505-509: The code assumes send_command returns a boolean "success"
but Requester.send_command() actually returns (cmd, ret/msg); change the
unpacking to "cmd, msg = await self.send_command(command)" and update the error
check to inspect the actual returned strings (e.g. treat an error if either cmd
or msg is a string starting with "$NK" or msg is None/empty) instead of using
"if not success"; update the log line to use the new variables (cmd, msg) and
return False on that condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 715e25bd-75be-42fd-9fb5-877c9cea3137
📒 Files selected for processing (7)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_coverage_gap.pytests/test_override.pytests/test_requester.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openevsehttp/override.py
- tests/test_override.py
…commands while adding a callback setter method
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_coverage_gap.py (2)
81-117: Test is missing cleanup and final newline.The test file ends abruptly without proper cleanup. Consider closing the charger's session if it was created internally, and ensure the file ends with a newline for consistency.
♻️ Suggested cleanup
with pytest.raises(UnknownError): await charger.clear_override() + + # Cleanup if needed + if hasattr(charger, '_session') and charger._session and not charger._session_external: + await charger._session.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_coverage_gap.py` around lines 81 - 117, The test leaves the OpenEVSE client's resources open and the file lacks a terminating newline; after the last assertion in test_override_failure_logic(), close the charger's session by calling the appropriate cleanup method on the OpenEVSE instance (e.g., await charger.close() if it provides an async close, or charger.close()/charger.session.close() for a sync close) to ensure no dangling connections, and add a final newline at EOF so the file ends with a newline character.
36-43: Async iterator side_effect should return an iterator, not be an async generator function.
mock_ws.__aiter__.side_effect = async_iter_stopassigns an async generator function, but__aiter__should return an async iterator object. When the code callsasync for message in ws_client, it invokesws_client.__aiter__()which will callside_effect()and return a new generator each time. This works by accident but is fragile.♻️ Cleaner pattern
- async def async_iter_stop(): - # Yield one message, then change state to STOPPED - yield msg - ws.state = STATE_STOPPED - yield msg # Should break before this is processed if we use it, - # but the iterator loop itself checks state - - mock_ws.__aiter__.side_effect = async_iter_stop + async def async_iter_stop(): + yield msg + ws.state = STATE_STOPPED + yield msg + + mock_ws.__aiter__ = lambda self: async_iter_stop()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_coverage_gap.py` around lines 36 - 43, async_iter_stop is an async generator function but you set mock_ws.__aiter__.side_effect to the function itself; __aiter__ must return an async iterator object, so change the assignment to return an instance of the async generator (e.g. set mock_ws.__aiter__.side_effect = lambda: async_iter_stop() or set mock_ws.__aiter__.return_value = async_iter_stop()) so that calls to ws_client.__aiter__ receive an async iterator object rather than the generator function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/requester.py`:
- Around line 129-136: The POST-response handling can reenter recursively via
Requester._update_callback (set by Requester.set_update_callback) because
update() calls process_request(); add a reentrancy guard on the Requester
instance (e.g., a boolean _invoking_update) and use it inside
_process_request_with_session: if _invoking_update is True skip/avoid awaiting
the callback, otherwise set _invoking_update = True, await
self._update_callback() in a try/finally and clear the flag in finally to
prevent nested invocations when a POST response contains "config_version" and
update() issues further POSTs.
---
Nitpick comments:
In `@tests/test_coverage_gap.py`:
- Around line 81-117: The test leaves the OpenEVSE client's resources open and
the file lacks a terminating newline; after the last assertion in
test_override_failure_logic(), close the charger's session by calling the
appropriate cleanup method on the OpenEVSE instance (e.g., await charger.close()
if it provides an async close, or charger.close()/charger.session.close() for a
sync close) to ensure no dangling connections, and add a final newline at EOF so
the file ends with a newline character.
- Around line 36-43: async_iter_stop is an async generator function but you set
mock_ws.__aiter__.side_effect to the function itself; __aiter__ must return an
async iterator object, so change the assignment to return an instance of the
async generator (e.g. set mock_ws.__aiter__.side_effect = lambda:
async_iter_stop() or set mock_ws.__aiter__.return_value = async_iter_stop()) so
that calls to ws_client.__aiter__ receive an async iterator object rather than
the generator function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a4ad0a8-9ff2-4fc6-8dfe-bc02eb9bb186
📒 Files selected for processing (7)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_coverage_gap.pytests/test_override.pytests/test_requester.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_override.py
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/test_override.py (1)
299-303:⚠️ Potential issue | 🟡 MinorMove these
caplogassertions outsidepytest.raises().Once
clear_override()/get_override()raisesUnsupportedFeature, control leaves the context immediately, so the assertions on Lines 303 and 330 never execute.🧪 Possible adjustment
- with pytest.raises(UnsupportedFeature): - with caplog.at_level(logging.DEBUG): - await test_charger_v2.update() - await test_charger_v2.clear_override() - assert "Feature not supported for older firmware." in caplog.text + await test_charger_v2.update() + with caplog.at_level(logging.DEBUG): + with pytest.raises(UnsupportedFeature): + await test_charger_v2.clear_override() + assert "Feature not supported for older firmware." in caplog.text @@ - with pytest.raises(UnsupportedFeature): - with caplog.at_level(logging.DEBUG): - await test_charger_v2.update() - await test_charger_v2.get_override() - assert "Feature not supported for older firmware." in caplog.text + await test_charger_v2.update() + with caplog.at_level(logging.DEBUG): + with pytest.raises(UnsupportedFeature): + await test_charger_v2.get_override() + assert "Feature not supported for older firmware." in caplog.textAlso applies to: 326-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_override.py` around lines 299 - 303, The test currently wraps the caplog assertions inside the pytest.raises(UnsupportedFeature) block so when test_charger_v2.clear_override() or get_override() raises the exception the rest of the assertions never run; refactor the test to limit the pytest.raises context to only the call that should raise (e.g., await test_charger_v2.clear_override() or await test_charger_v2.get_override()), then use a separate with caplog.at_level(logging.DEBUG): block after the raises context to perform the assert "Feature not supported for older firmware." in caplog.text (and do the same for the other occurrence around test_charger_v2.update()/get_override()).tests/test_requester.py (1)
35-39:⚠️ Potential issue | 🟡 MinorReplace the tautological post-error assertion.
assert test_charger_auth_err is not Nonewill always pass for a constructed fixture, so it doesn't verify the auth-failure path. Assert an observable postcondition instead, e.g. that the failed update left the cached state untouched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_requester.py` around lines 35 - 39, The test currently ends with a tautological assertion; instead capture the pre-update cached state from the fixture (e.g. pre_state = test_charger_auth_err.state or test_charger_auth_err.cached_status) before awaiting update(), then inside the pytest.raises block verify the AuthenticationError is thrown and afterwards assert that test_charger_auth_err.state (or the fixture's cached status field) is equal to pre_state to ensure the failed update did not mutate cached state; update the test_get_status_auth_err to use the actual cached/state attribute name present on test_charger_auth_err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 129-138: The code is replacing the live caches _status/_config
with error envelopes returned by process_request; update the loop that calls
self.process_request(url, method="get") to check the response for an error
envelope (e.g., if isinstance(response, dict) and response.get("ok") is False)
and skip mutating self._status or self._config in that case (log the error) so
the previous good snapshot is retained; only assign to self._status when the url
contains "/status" and the response is a successful payload, and similarly only
assign to self._config for non-status URLs when the response is successful.
- Around line 259-260: The receive loop currently awaits self.update() when any
key in UPDATE_TRIGGERS is present, but any exception (timeout/auth/etc.) will
bubble up and kill the listener task; wrap the await self.update() call in a
try/except that catches expected errors (e.g., asyncio.TimeoutError,
authentication errors, and a general Exception fallback), log the failure with
context (including which trigger keys caused it) and swallow the exception so
the websocket listener (see openevsehttp/websocket.py awaiting _update_status())
continues processing future pushes; keep the update() retry/propagation logic
unchanged outside this receive path.
- Around line 140-144: The update() path currently constructs OpenEVSEWebsocket
(calling OpenEVSEWebsocket.__init__) which eagerly allocates an aiohttp
ClientSession and leaks a session for polling-only clients; instead, remove the
direct instantiation in update() and use the existing lazy path
(_start_listening()) or defer creation until ws_connect/ws_listen is actually
needed so the websocket attribute remains None for polling-only clients; ensure
ws_disconnect() still properly cleans up if a session is later created.
In `@openevsehttp/requester.py`:
- Around line 157-160: The code flattens the RAPI reply and discards transport
errors returned by process_request (which can be {"ok": False, ...}); update the
block around process_request / value so that if value.get("ok") is False you
return value unchanged (preserving the transport failure/status), otherwise
proceed to extract cmd = value.get("cmd", command) and return the existing (cmd,
value.get("msg","")) form; reference the variables/process_request function and
the value/cmd handling to locate the change.
---
Duplicate comments:
In `@tests/test_override.py`:
- Around line 299-303: The test currently wraps the caplog assertions inside the
pytest.raises(UnsupportedFeature) block so when test_charger_v2.clear_override()
or get_override() raises the exception the rest of the assertions never run;
refactor the test to limit the pytest.raises context to only the call that
should raise (e.g., await test_charger_v2.clear_override() or await
test_charger_v2.get_override()), then use a separate with
caplog.at_level(logging.DEBUG): block after the raises context to perform the
assert "Feature not supported for older firmware." in caplog.text (and do the
same for the other occurrence around test_charger_v2.update()/get_override()).
In `@tests/test_requester.py`:
- Around line 35-39: The test currently ends with a tautological assertion;
instead capture the pre-update cached state from the fixture (e.g. pre_state =
test_charger_auth_err.state or test_charger_auth_err.cached_status) before
awaiting update(), then inside the pytest.raises block verify the
AuthenticationError is thrown and afterwards assert that
test_charger_auth_err.state (or the fixture's cached status field) is equal to
pre_state to ensure the failed update did not mutate cached state; update the
test_get_status_auth_err to use the actual cached/state attribute name present
on test_charger_auth_err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34e4e627-6b82-4233-843f-bea1800c7313
📒 Files selected for processing (7)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_coverage_gap.pytests/test_override.pytests/test_requester.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openevsehttp/override.py
…RAPI command execution
…tests Signed-off-by: firstof9@gmail.com <firstof9@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_client.py (1)
1398-1401:⚠️ Potential issue | 🟡 MinorMove these
caplogassertions out of thepytest.raises()blocks.Once the awaited call raises, execution exits the context manager immediately, so these assertions never run and the logging paths stay unverified.
Also applies to: 1556-1559, 1566-1570, 1595-1598, 1621-1624, 1680-1685
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 1398 - 1401, The caplog assertions are inside the pytest.raises(...) context so they never run after the awaited call raises; move the assertion(s) that check caplog.text out of the with pytest.raises(...) block (but keep them inside the with caplog.at_level(logging.ERROR) context) so they execute after await charger.set_service_level(1) raises; apply the same change to the other similar cases flagged (the other with pytest.raises blocks around awaited charger.* calls) so the logging assertions run and verify the logged error messages.
🧹 Nitpick comments (1)
tests/test_client.py (1)
1547-1551: Stub the websocket coroutines in these unit tests.
ws_start()schedules reallisten()/keepalive()tasks fromOpenEVSE._start_listening(). Leaving those running while the test asserts unrelated branches makes the cases timing-dependent and can leak background websocket work into later assertions. Patch the websocket methods or inject a fake websocket here.Also applies to: 1733-1744
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 1547 - 1551, The tests call charger.ws_start() which triggers OpenEVSE._start_listening() to schedule real listen() and keepalive() tasks; stub those coroutines or inject a fake websocket on the charger before calling ws_start() so no background tasks run: e.g., monkeypatch the OpenEVSE.listen and OpenEVSE.keepalive coroutines to async no-ops or set charger._ws to a simple fake object with the same interface, then call ws_start() and ws_disconnect() as before to assert logs—ensure the stubbed methods are applied for the charger instance used in the test (refer to functions ws_start, ws_disconnect, and method OpenEVSE._start_listening and coroutines listen/keepalive).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 159-165: The code is treating any non-dict or
dict-without-wifi_serial as an older firmware and raising MissingSerial; first
detect and propagate HTTP/request errors returned by process_request() by
checking if response is a dict with an "ok" key set to False and then
raise/return a request-specific error (include response["status"] or the whole
response in the error/log) instead of collapsing into MissingSerial; only if the
response is a successful config payload (e.g., dict with "ok" True or not an
error envelope) and wifi_serial is missing should you raise MissingSerial —
update the logic around process_request() / serial extraction to check
response.get("ok") is False first and handle that error path, otherwise proceed
to check for wifi_serial.
In `@openevsehttp/requester.py`:
- Around line 130-141: The post-write refresh can raise and make a successful
POST look like it failed; inside the block that checks method == "post" and
"config_version" and calls await self._update_callback() (guarded by
self._invoking_callback), wrap the await in a try/except that catches all
exceptions from the refresh, logs the error (using the instance logger or
logging.exception) and suppresses the exception so the original POST result is
returned to callers; keep setting and clearing self._invoking_callback in the
same finally behavior.
---
Duplicate comments:
In `@tests/test_client.py`:
- Around line 1398-1401: The caplog assertions are inside the pytest.raises(...)
context so they never run after the awaited call raises; move the assertion(s)
that check caplog.text out of the with pytest.raises(...) block (but keep them
inside the with caplog.at_level(logging.ERROR) context) so they execute after
await charger.set_service_level(1) raises; apply the same change to the other
similar cases flagged (the other with pytest.raises blocks around awaited
charger.* calls) so the logging assertions run and verify the logged error
messages.
---
Nitpick comments:
In `@tests/test_client.py`:
- Around line 1547-1551: The tests call charger.ws_start() which triggers
OpenEVSE._start_listening() to schedule real listen() and keepalive() tasks;
stub those coroutines or inject a fake websocket on the charger before calling
ws_start() so no background tasks run: e.g., monkeypatch the OpenEVSE.listen and
OpenEVSE.keepalive coroutines to async no-ops or set charger._ws to a simple
fake object with the same interface, then call ws_start() and ws_disconnect() as
before to assert logs—ensure the stubbed methods are applied for the charger
instance used in the test (refer to functions ws_start, ws_disconnect, and
method OpenEVSE._start_listening and coroutines listen/keepalive).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03909a13-67ba-481a-9bb8-3a4cfdc89a82
📒 Files selected for processing (8)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/test_client.pytests/test_coverage_gap.pytests/test_external_session.pytests/test_override.pytests/test_requester.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_override.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_external_session.py
- openevsehttp/override.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_override.py (1)
143-149:⚠️ Potential issue | 🟡 MinorMove the
caplogassertions below thepytest.raises()blocks.The same dead-code pattern is still present in these new cases: once the awaited call raises, control exits the
pytest.raises()body immediately, so theassert ... in caplog.textlines never run. These branches are checking the exception, but not the logging they claim to cover.One fix pattern
with caplog.at_level(logging.ERROR): with pytest.raises(UnknownError): await test_charger_legacy.toggle_override() assert "Problem toggling override ..." in caplog.textAlso applies to: 163-166, 181-187, 224-227, 241-244, 361-378, 396-399, 423-426, 486-503, 531-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_override.py` around lines 143 - 149, The caplog assertions are placed inside the pytest.raises() context so they never run after the awaited call raises; move the caplog checks out of the inner with pytest.raises(...) blocks so they execute after the exception is caught. For each occurrence (e.g., the block wrapping await test_charger_legacy.toggle_override()), change the structure to open caplog.at_level(...), then open pytest.raises(...) around the await, close pytest.raises(...), and then assert the expected message is in caplog.text (do the same refactor for the other listed blocks such as the ones around toggle_override() and similar awaits at lines 163-166, 181-187, 224-227, 241-244, 361-378, 396-399, 423-426, 486-503, 531-534).
🧹 Nitpick comments (3)
openevsehttp/requester.py (1)
136-148: Extract the refresh-trigger keys into a shared constant.This condition duplicates the trigger list already defined as
UPDATE_TRIGGERSinopenevsehttp/client.py. If one side changes and the other does not, websocket-triggered refreshes and write-triggered refreshes will drift. Please move the list to a neutral shared module and reuse it from both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 136 - 148, The repeated list of refresh-trigger keys (used in the condition inside requester.py and the existing UPDATE_TRIGGERS in client.py) should be moved to a single shared constant in a neutral module and both files should import it; create a new module that exports the list (e.g., UPDATE_TRIGGERS or TRIGGER_KEYS), replace the hard-coded tuple check in requester.py (the conditional using method and message) to reference the imported constant, and update openevsehttp/client.py to import the same constant instead of its local UPDATE_TRIGGERS so both places use the identical source of truth.openevsehttp/client.py (2)
175-179: Redundant type check.The
isinstance(response, dict)check on Line 177 is redundant since Line 162 already ensuresresponseis a dict (raisesUnknownErrorotherwise).Simplify model extraction
- model = ( - response.get("buildenv", "unknown") - if isinstance(response, dict) - else "unknown" - ) + model = response.get("buildenv", "unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 175 - 179, The model extraction uses a redundant isinstance(response, dict) guard; since earlier code already validates that response is a dict (and raises on error), simplify the assignment in the model extraction (the code that assigns to model from response.get("buildenv", "unknown")) by removing the conditional branch and directly using response.get("buildenv", "unknown"). Update the expression that assigns to model so it relies on the validated response variable rather than re-checking its type.
201-207:asyncio.get_event_loop()is deprecated when no loop is running in Python 3.10+.Since this project targets Python 3.13+, using
asyncio.get_event_loop()in the except block will emit a deprecation warning. Replace it withasyncio.new_event_loop()followed byasyncio.set_event_loop()to create a new event loop explicitly.Suggested fix
if not self._loop: try: _LOGGER.debug("Attempting to find running loop...") self._loop = asyncio.get_running_loop() except RuntimeError: - self._loop = asyncio.get_event_loop() - _LOGGER.debug("Using new event loop...") + self._loop = asyncio.new_event_loop() + asyncio.set_event_loop(self._loop) + _LOGGER.debug("Created and set new event loop...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 201 - 207, The current fallback uses deprecated asyncio.get_event_loop(); in the except block create a new loop with asyncio.new_event_loop(), call asyncio.set_event_loop(new_loop), and assign that new_loop to self._loop (and keep the existing debug log). Locate the code that checks/assigns self._loop (the try/except block around asyncio.get_running_loop()) and replace the get_event_loop() call with creating and setting a new event loop as described.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 527-538: The code path guarded by _version_check("4.1.2") accesses
self._config["min_current_hard"] and ["max_current_hard"] directly which can
raise KeyError if update() hasn't populated those keys; change the checks in
that block (inside the _version_check branch where amps is validated and
set_override is called) to use safe lookups (e.g.
self._config.get("min_current_hard") / .get("max_current_hard")) with sensible
defaults or raise a clear ValueError/RuntimeError if the config values are
missing, and ensure the error log includes context before raising so the
function fails gracefully instead of raising KeyError.
In `@openevsehttp/override.py`:
- Around line 146-150: In clear(), mirror the failure-handling used in
set()/toggle(): after awaiting self._evse.process_request, check both
response.get("ok", True) and the response "msg" payload (extract via msg =
response.get("msg") if isinstance(response, dict) else response) and treat msg
values like "failed" or "error" (or any non-success sentinel used by
set()/toggle()) as a failure—log the full response and raise UnknownError when
either ok is false or msg indicates failure so DELETE failures are rejected the
same way as other override writes.
---
Duplicate comments:
In `@tests/test_override.py`:
- Around line 143-149: The caplog assertions are placed inside the
pytest.raises() context so they never run after the awaited call raises; move
the caplog checks out of the inner with pytest.raises(...) blocks so they
execute after the exception is caught. For each occurrence (e.g., the block
wrapping await test_charger_legacy.toggle_override()), change the structure to
open caplog.at_level(...), then open pytest.raises(...) around the await, close
pytest.raises(...), and then assert the expected message is in caplog.text (do
the same refactor for the other listed blocks such as the ones around
toggle_override() and similar awaits at lines 163-166, 181-187, 224-227,
241-244, 361-378, 396-399, 423-426, 486-503, 531-534).
---
Nitpick comments:
In `@openevsehttp/client.py`:
- Around line 175-179: The model extraction uses a redundant
isinstance(response, dict) guard; since earlier code already validates that
response is a dict (and raises on error), simplify the assignment in the model
extraction (the code that assigns to model from response.get("buildenv",
"unknown")) by removing the conditional branch and directly using
response.get("buildenv", "unknown"). Update the expression that assigns to model
so it relies on the validated response variable rather than re-checking its
type.
- Around line 201-207: The current fallback uses deprecated
asyncio.get_event_loop(); in the except block create a new loop with
asyncio.new_event_loop(), call asyncio.set_event_loop(new_loop), and assign that
new_loop to self._loop (and keep the existing debug log). Locate the code that
checks/assigns self._loop (the try/except block around
asyncio.get_running_loop()) and replace the get_event_loop() call with creating
and setting a new event loop as described.
In `@openevsehttp/requester.py`:
- Around line 136-148: The repeated list of refresh-trigger keys (used in the
condition inside requester.py and the existing UPDATE_TRIGGERS in client.py)
should be moved to a single shared constant in a neutral module and both files
should import it; create a new module that exports the list (e.g.,
UPDATE_TRIGGERS or TRIGGER_KEYS), replace the hard-coded tuple check in
requester.py (the conditional using method and message) to reference the
imported constant, and update openevsehttp/client.py to import the same constant
instead of its local UPDATE_TRIGGERS so both places use the identical source of
truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cbdfb73-a3a7-46c9-81c8-3e90e083a68d
📒 Files selected for processing (11)
openevsehttp/client.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (3)
- tests/test_external_session.py
- tests/fixtures/v_legacy_json/status.json
- tests/fixtures/v_legacy_json/config.json
…date override error detection logic
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openevsehttp/client.py (1)
876-881:⚠️ Potential issue | 🟡 MinorReject boolean ambient temperatures like the other temperature accessors.
boolis a subtype ofint, soFalse/Truecurrently become0.0/0.1here. The adjacent temperature properties already treat boolean payloads as invalid, so this path should not convert malformed sensor data into a real reading.Minimal fix
temp = self._status.get("temp") - if temp is not None: + if temp is not None and not isinstance(temp, bool): return temp / 10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 876 - 881, The ambient_temperature method currently treats bools as numeric (since bool is an int subtype) and thus converts True/False into temperatures; update ambient_temperature to mirror the other temperature accessors by rejecting boolean payloads: for both temp and temp1 check that the value is not None and not an instance of bool (or explicitly check for numeric types excluding bool) before dividing by 10 (e.g., if temp is not None and not isinstance(temp, bool): return temp / 10; then same for temp1); if neither value is a valid non-bool number, return None instead of converting a boolean to 0.0/0.1.
🧹 Nitpick comments (2)
tests/test_client.py (1)
473-530: Use boolean expectations for these boolean-accessor tests.These accessors now return real
bools, but comparing them to0/1with==still passes if they regress to raw integers becauseFalse == 0andTrue == 1. Switching these parametrized expectations toTrue/Falseand asserting withiswill actually lock in the casting behavior this PR introduced.Example pattern
- "fixture, expected", [("test_charger", 0), ("test_charger_v2", 0)] + "fixture, expected", [("test_charger", False), ("test_charger_v2", False)] ... - assert status == expected + assert status is expected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 473 - 530, Update the boolean expectations in the parametrized tests (test_get_tempt, test_get_diodet, test_get_ventt, test_get_groundt, test_get_relayt) to use True/False instead of 1/0 and change the assertions to use identity comparison (use "is" with the accessor values temp_check_enabled, diode_check_enabled, vent_required_enabled, ground_check_enabled, stuck_relay_check_enabled) so the tests verify actual bools rather than integer-equivalent values.tests/test_override.py (1)
298-359: Refactortest_set_overrideinto parameterized cases for better failure isolation.This block bundles many payload variants into one scenario, so one failure short-circuits later checks and makes diagnosis slower. Splitting into
pytest.mark.parametrizecases would keep each payload assertion independent and reduce repetitive setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_override.py` around lines 298 - 359, Refactor the long grouped assertions in the test function (test_set_override) into multiple pytest.mark.parametrize cases: create a parameterized test that calls test_charger.set_override with tuples of (args, kwargs, expected_log_substring) and in each param case register mock_aioclient.post(TEST_URL_OVERRIDE, status=200, body='{"msg": "OK"}'), call test_charger.set_override with the provided args/kwargs, assert the returned status, and assert the expected_log_substring is in caplog.text (clearing caplog between cases); this isolates each payload scenario, keeps use of test_charger.set_override, mock_aioclient.post and TEST_URL_OVERRIDE intact, and removes the single long sequence of assertions so failures map to a single param case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/requester.py`:
- Around line 136-159: The cache-refresh gating currently only checks
UPDATE_TRIGGERS and misses successful write responses like {"msg":"done"},
causing stale reads; update the conditional around method, message, and
UPDATE_TRIGGERS so that writes (method in ("post","patch","put")) also trigger
the _update_callback when the response indicates success (e.g.,
message.get("msg") == "done") or any UPDATE_TRIGGERS are present, while keeping
the existing _invoking_callback/_pending_refresh loop and _invoking_callback
flag handling intact; reference UPDATE_TRIGGERS, _update_callback,
_invoking_callback, and _pending_refresh (and caller methods such as
OpenEVSE.set_charge_mode / set_service_level / set_led_brightness) when making
the change.
In `@tests/test_override.py`:
- Around line 27-38: The test_toggle_override mocks both PATCH and POST (and
extra GETs) on the same /override URL which masks HTTP-method regressions;
update the mocks in test_toggle_override to only stub the exact HTTP method used
by the code under test (PATCH for the v4 toggle path) and remove the redundant
POST/GET stubs so the test fails if a POST/GET is accidentally sent; apply the
same tightening to the other override tests mentioned (the tests around the
other locations) so each test only registers the HTTP methods the implementation
should call.
---
Duplicate comments:
In `@openevsehttp/client.py`:
- Around line 876-881: The ambient_temperature method currently treats bools as
numeric (since bool is an int subtype) and thus converts True/False into
temperatures; update ambient_temperature to mirror the other temperature
accessors by rejecting boolean payloads: for both temp and temp1 check that the
value is not None and not an instance of bool (or explicitly check for numeric
types excluding bool) before dividing by 10 (e.g., if temp is not None and not
isinstance(temp, bool): return temp / 10; then same for temp1); if neither value
is a valid non-bool number, return None instead of converting a boolean to
0.0/0.1.
---
Nitpick comments:
In `@tests/test_client.py`:
- Around line 473-530: Update the boolean expectations in the parametrized tests
(test_get_tempt, test_get_diodet, test_get_ventt, test_get_groundt,
test_get_relayt) to use True/False instead of 1/0 and change the assertions to
use identity comparison (use "is" with the accessor values temp_check_enabled,
diode_check_enabled, vent_required_enabled, ground_check_enabled,
stuck_relay_check_enabled) so the tests verify actual bools rather than
integer-equivalent values.
In `@tests/test_override.py`:
- Around line 298-359: Refactor the long grouped assertions in the test function
(test_set_override) into multiple pytest.mark.parametrize cases: create a
parameterized test that calls test_charger.set_override with tuples of (args,
kwargs, expected_log_substring) and in each param case register
mock_aioclient.post(TEST_URL_OVERRIDE, status=200, body='{"msg": "OK"}'), call
test_charger.set_override with the provided args/kwargs, assert the returned
status, and assert the expected_log_substring is in caplog.text (clearing caplog
between cases); this isolates each payload scenario, keeps use of
test_charger.set_override, mock_aioclient.post and TEST_URL_OVERRIDE intact, and
removes the single long sequence of assertions so failures map to a single param
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45113fdd-76d8-4a2f-bb7d-67b92dacaca9
📒 Files selected for processing (12)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/v_legacy_json/config.json
- openevsehttp/const.py
- tests/fixtures/v_legacy_json/status.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_external_session.py
- openevsehttp/override.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openevsehttp/requester.py (1)
100-103: Passing bothdataandjsonkwargs simultaneously whenrapiis provided.When
rapiis not None,kwargsis initialized with{"data": rapi, ...}. Ifdatais also provided (which shouldn't happen due to line 65-68 validation), the subsequentif data is not None: kwargs["json"] = datawould add ajsonkey. However, since line 65-68 prevents this, the code is safe.But there's a subtle issue: when
rapiis None,kwargs["data"]is set toNone, which aiohttp may interpret differently than omitting the key entirely.♻️ Consider omitting None data key
try: - kwargs = {"data": rapi, "auth": auth} + kwargs: dict[str, Any] = {"auth": auth} + if rapi is not None: + kwargs["data"] = rapi if data is not None: kwargs["json"] = data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 100 - 103, The request building passes kwargs = {"data": rapi, "auth": auth} which leaves data=None in the dict when rapi is None; change the logic in the requester.py block around kwargs/http_method so you only add the "data" key when rapi is not None (e.g., conditionally set kwargs["data"]=rapi) and keep the existing conditional that adds kwargs["json"]=data only when data is provided; this removes the spurious data=None entry that aiohttp may treat differently while preserving current validation that prevents both body types being set.tests/conftest.py (1)
208-222: Consider adding websocket mock for consistency.The
test_charger_legacyfixture doesn't mock the websocket endpoint (TEST_URL_WS), unlike other fixtures liketest_charger(lines 65-70). While this may be intentional since legacy firmware tests don't exercise websocket features, it could cause unexpected behavior if a test accidentally triggers websocket initialization.♻️ Add websocket mock for consistency
`@pytest.fixture`(name="test_charger_legacy") def test_charger_legacy(mock_aioclient): """Load the charger data for legacy firmware.""" mock_aioclient.get( TEST_URL_STATUS, status=200, body=load_fixture("v_legacy_json/status.json"), ) mock_aioclient.get( TEST_URL_CONFIG, status=200, body=load_fixture("v_legacy_json/config.json"), ) + mock_aioclient.get( + TEST_URL_WS, + status=200, + body=load_fixture("v_legacy_json/status.json"), + repeat=True, + ) return main.OpenEVSE(TEST_TLD)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 208 - 222, Add a websocket mock to the test_charger_legacy fixture: before returning main.OpenEVSE(TEST_TLD) call mock_aioclient.get for TEST_URL_WS (same pattern as in test_charger) so the websocket endpoint is stubbed (e.g. mock_aioclient.get(TEST_URL_WS, status=200, body=load_fixture("v_legacy_json/ws.json")) or body="" if no fixture), keeping the rest of test_charger_legacy (mock_aioclient, TEST_URL_STATUS, TEST_URL_CONFIG) unchanged.openevsehttp/override.py (1)
119-122: Consider documenting the magic number 254.The state code
254represents "sleeping" according toopenevsehttp/client.py:53, but this context isn't immediately obvious here. A brief comment would improve maintainability.📝 Add clarifying comment
_LOGGER.debug( "Toggling manual override via RAPI. Current state: %s", int_state ) + # State 254 = "sleeping" - use $FE to enable, otherwise $FS to sleep command = "$FE" if int_state == 254 else "$FS"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/override.py` around lines 119 - 122, The conditional uses the magic number 254 for int_state without context; document it by either replacing the literal with a named constant (e.g., STATE_SLEEPING) imported from the client module or add a one-line comment above the command assignment explaining that 254 represents the "sleeping" state (as defined in client.py), so _LOGGER.debug, int_state and command = "$FE" if int_state == 254 else "$FS" are clear and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openevsehttp/override.py`:
- Around line 119-122: The conditional uses the magic number 254 for int_state
without context; document it by either replacing the literal with a named
constant (e.g., STATE_SLEEPING) imported from the client module or add a
one-line comment above the command assignment explaining that 254 represents the
"sleeping" state (as defined in client.py), so _LOGGER.debug, int_state and
command = "$FE" if int_state == 254 else "$FS" are clear and maintainable.
In `@openevsehttp/requester.py`:
- Around line 100-103: The request building passes kwargs = {"data": rapi,
"auth": auth} which leaves data=None in the dict when rapi is None; change the
logic in the requester.py block around kwargs/http_method so you only add the
"data" key when rapi is not None (e.g., conditionally set kwargs["data"]=rapi)
and keep the existing conditional that adds kwargs["json"]=data only when data
is provided; this removes the spurious data=None entry that aiohttp may treat
differently while preserving current validation that prevents both body types
being set.
In `@tests/conftest.py`:
- Around line 208-222: Add a websocket mock to the test_charger_legacy fixture:
before returning main.OpenEVSE(TEST_TLD) call mock_aioclient.get for TEST_URL_WS
(same pattern as in test_charger) so the websocket endpoint is stubbed (e.g.
mock_aioclient.get(TEST_URL_WS, status=200,
body=load_fixture("v_legacy_json/ws.json")) or body="" if no fixture), keeping
the rest of test_charger_legacy (mock_aioclient, TEST_URL_STATUS,
TEST_URL_CONFIG) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7423779-3ca2-484b-8801-6320a2b64a20
📒 Files selected for processing (12)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (4)
- tests/fixtures/v_legacy_json/status.json
- tests/fixtures/v_legacy_json/config.json
- tests/test_requester.py
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openevsehttp/const.py
… update test fixtures
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_override.py (1)
27-38:⚠️ Potential issue | 🟡 MinorOnly stub the verb this toggle path is supposed to use.
Accepting both
PATCH /overrideandPOST /overridemakes this test pass even iftoggle_override()regresses to the wrong HTTP method. Tightening the mock to the expected verb keeps the test guarding the contract.🧪 Suggested tightening
mock_aioclient.patch( TEST_URL_OVERRIDE, status=200, body="OK", repeat=True, ) - mock_aioclient.post( - TEST_URL_OVERRIDE, - status=200, - body='{"msg": "OK"}', - repeat=True, - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_override.py` around lines 27 - 38, The test currently stubs both mock_aioclient.patch and mock_aioclient.post for TEST_URL_OVERRIDE which hides regressions in the HTTP verb used by toggle_override(); update the test so only the expected verb is stubbed (remove the extraneous mock_aioclient.post or replace it with the correct verb) and ensure the stubbed URL/response matches the behavior of toggle_override() so the test fails if the function switches to the wrong HTTP method.openevsehttp/requester.py (1)
138-143:⚠️ Potential issue | 🟠 MajorTrigger the refresh callback for successful message-only writes too.
set_charge_mode(),set_service_level(), andset_led_brightness()all treat{"msg": "done"}/{"msg": "no change"}as success and do not update_configlocally. With the current predicate, those calls can return successfully whilecharge_mode,service_level, andled_brightnessstay stale until some later manualupdate()or websocket push.♻️ One centralized way to fix it
- if ( - method in ("post", "patch", "put") - and isinstance(message, dict) - and any(key in message for key in UPDATE_TRIGGERS) - and self._update_callback - ): + if ( + method in ("post", "patch", "put") + and isinstance(message, dict) + and message.get("ok", True) is not False + and self._update_callback + and ( + any(key in message for key in UPDATE_TRIGGERS) + or message.get("msg") in {"done", "no change"} + ) + ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 138 - 143, The current predicate that triggers self._update_callback only when any key from UPDATE_TRIGGERS is present misses successful write responses that return {"msg": "done"} or {"msg": "no change"}, causing stale values for charge_mode, service_level, and led_brightness; update the condition in requester.py so that for write methods ("post","patch","put") you also trigger self._update_callback when message is a dict and message.get("msg") is one of the success tokens (e.g. "done", "no change") in addition to the existing any(key in message for key in UPDATE_TRIGGERS), thereby ensuring set_charge_mode, set_service_level, and set_led_brightness cause a refresh when the device responds with a success message.
🧹 Nitpick comments (1)
tests/test_requester.py (1)
28-28: Consider replacing repeated placeholder docstrings with test-specific intent.Several functions still use
"Test v4 Status reply.", which makes failures harder to triage quickly.Also applies to: 36-36, 44-44, 56-56, 68-68, 80-80, 92-92, 135-135, 145-145, 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_requester.py` at line 28, Multiple test functions in tests/test_requester.py use the repeated placeholder docstring "Test v4 Status reply." which makes failures ambiguous; locate every test whose first statement is the exact string "Test v4 Status reply." and replace it with a concise, test-specific docstring that states the intent (e.g., "Verify 4xx status returns X and error message Y" or "Ensure POST /endpoint returns 200 and correct payload"); keep the docstring one line, reflect the assertion being made, and update all occurrences (search for the literal "Test v4 Status reply.") so each test function clearly communicates what it verifies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_external_session.py`:
- Around line 118-130: The test currently patches OpenEVSE.repeat but leaves
ws_start() to schedule self.websocket.listen(), which can race with
ws_disconnect(); patch the websocket listener as well by mocking the WebSocket
listen coroutine used by OpenEVSE so no real network is attempted — e.g., patch
the OpenEVSE.websocket.listen (or the concrete WebSocketClient.listen used by
ws_start) with an AsyncMock before calling charger = OpenEVSE(...) and await
charger.ws_start(), then assert charger.websocket and its
session/_session_external as before and await charger.ws_disconnect().
---
Duplicate comments:
In `@openevsehttp/requester.py`:
- Around line 138-143: The current predicate that triggers self._update_callback
only when any key from UPDATE_TRIGGERS is present misses successful write
responses that return {"msg": "done"} or {"msg": "no change"}, causing stale
values for charge_mode, service_level, and led_brightness; update the condition
in requester.py so that for write methods ("post","patch","put") you also
trigger self._update_callback when message is a dict and message.get("msg") is
one of the success tokens (e.g. "done", "no change") in addition to the existing
any(key in message for key in UPDATE_TRIGGERS), thereby ensuring
set_charge_mode, set_service_level, and set_led_brightness cause a refresh when
the device responds with a success message.
In `@tests/test_override.py`:
- Around line 27-38: The test currently stubs both mock_aioclient.patch and
mock_aioclient.post for TEST_URL_OVERRIDE which hides regressions in the HTTP
verb used by toggle_override(); update the test so only the expected verb is
stubbed (remove the extraneous mock_aioclient.post or replace it with the
correct verb) and ensure the stubbed URL/response matches the behavior of
toggle_override() so the test fails if the function switches to the wrong HTTP
method.
---
Nitpick comments:
In `@tests/test_requester.py`:
- Line 28: Multiple test functions in tests/test_requester.py use the repeated
placeholder docstring "Test v4 Status reply." which makes failures ambiguous;
locate every test whose first statement is the exact string "Test v4 Status
reply." and replace it with a concise, test-specific docstring that states the
intent (e.g., "Verify 4xx status returns X and error message Y" or "Ensure POST
/endpoint returns 200 and correct payload"); keep the docstring one line,
reflect the assertion being made, and update all occurrences (search for the
literal "Test v4 Status reply.") so each test function clearly communicates what
it verifies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9ccdb5d-5435-4fae-b7bf-7f46573c1065
📒 Files selected for processing (12)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/v_legacy_json/config.json
- openevsehttp/const.py
- tests/fixtures/v_legacy_json/status.json
🚧 Files skipped from review as they are similar to previous changes (1)
- openevsehttp/override.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Line 91: The current single update() method is too narrow: when _ws_listening
is True it only reloads /config and leaves status-backed fields stale; change
the callback wiring so websocket UPDATE_TRIGGERS and
requester.set_update_callback use a forced/full refresh path instead of the
optimized poll-only path. Implement either a new full_refresh() (or
update(force_full=True)) in the same class and ensure
set_update_callback(self.full_refresh) and the UPDATE_TRIGGERS branch call
full_refresh (or call update with force_full=True); inside full_refresh load
both /config and the status endpoints (ensuring state, pilot, manual_override
and override_version/limit_version are refreshed) while leaving the existing
optimized update behavior for regular polling.
In `@openevsehttp/requester.py`:
- Around line 138-146: The success-detection guard around coalesced refresh
skips legacy RAPI replies (e.g., {"ret":"$OK"} or {"cmd":...}) so
_update_callback never runs after send_command() and legacy methods like
toggle_override()/set_current(); update the condition in the method/message
check to also treat message.get("ret") == "$OK" and presence of a "cmd" key as
success (i.e., OR in message.get("ret") in ("$OK", ...) or message.get("cmd") is
not None), and make the same change in the similar guard around the second block
(the other occurrence noted) so legacy replies trigger _update_callback and
refresh the cache.
In `@tests/test_override.py`:
- Around line 17-29: The test is masking refresh failures because
Requester._update_callback() runs after successful PATCH/POST and consumes the
already-used shared /status and /config stubs; to fix, either (a) add
repeat=True to the shared GET stubs for /status and /config used by
test_toggle_override_base (and the other listed tests) so the follow-up refresh
GETs succeed, or (b) explicitly disable the refresh callback before invoking the
write (e.g., set test_charger.requester._update_callback = None) so only the
PATCH behavior is exercised; locate spots referencing test_charger.update(),
test_charger.toggle_override(), and Requester._update_callback() to apply the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2d92714-ea52-4ebe-b0c7-10d545d08e92
📒 Files selected for processing (13)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (5)
- tests/fixtures/v_legacy_json/status.json
- openevsehttp/const.py
- tests/fixtures/v_legacy_json/config.json
- tests/test_client.py
- tests/test_requester.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_external_session.py
- tests/test_firmware.py
- openevsehttp/override.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
openevsehttp/override.py (1)
147-155: Redundantisinstancecheck after dict assumption.Line 148 calls
response.get("ok", True)which assumesresponseis a dict. If this succeeds, theisinstance(response, dict)check at line 155 is always true. However,process_requestmay return non-dict values in edge cases (e.g., error envelopes normalized from plain text). The current code is defensive but slightly inconsistent.For consistency with the error check pattern, consider handling the non-dict case before calling
.get():♻️ Suggested minor refactor
response = await self._evse.process_request(url=url, method="delete") + if not isinstance(response, dict): + _LOGGER.error("Problem clearing override. Response: %s", response) + raise UnknownError if not response.get("ok", True) or response.get("msg") in ( "failed", "error", ): _LOGGER.error("Problem clearing override. Response: %s", response) raise UnknownError - msg = response.get("msg") if isinstance(response, dict) else response + msg = response.get("msg", response) _LOGGER.debug("Clear response: %s", msg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/override.py` around lines 147 - 155, The code assumes response is a dict when calling self._evse.process_request and using response.get(...), but later checks isinstance(response, dict) for msg extraction; refactor by first checking if response is a dict right after response = await self._evse.process_request(...) (use isinstance(response, dict)), handle the non-dict case consistently (e.g., log the raw response via _LOGGER.error or set msg = response and raise UnknownError as appropriate), and then safely call response.get("ok", True) and response.get("msg") for the dict path so the subsequent msg = response.get("msg") logic (and the UnknownError raise) no longer relies on a redundant isinstance check. Ensure references to _evse.process_request, response, _LOGGER, and UnknownError are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 126-139: The code assigns response directly to self._status /
self._config even when response is not a dict; update the logic in the block
that calls process_request (the code handling response, response.get(...),
"/status" check) to first verify isinstance(response, dict) before assigning to
self._status or self._config, otherwise skip the assignment and log a
debug/warning about unexpected non-dict payloads; use the existing response
shape checks and update the branches around response, self._status, self._config
and the "/status" test to enforce the dict guard.
In `@tests/test_client.py`:
- Around line 2296-2304: The test incorrectly initializes charger.tasks as a
list; change the assignment at the top of the snippet to use a set (i.e., set
literal containing task) so charger.tasks = {task} instead of charger.tasks =
[task]; this keeps subsequent code that iterates, cancels, and gathers tasks
(including assertions about length and calls to charger.ws_start) correct since
charger.tasks is expected to be a set by the implementation (refer to
charger.tasks and ws_start).
- Around line 2207-2223: The test assigns charger.tasks to a list ([task]) but
OpenEVSE initializes tasks as a set(), causing a type mismatch; update the test
in test_client.py (function test_client_ws_disconnect_task_cleanup) to assign a
set instead (e.g., charger.tasks = {task}) so the test uses the same container
type as OpenEVSE and any set operations like .add()/.remove() behave correctly
when ws_disconnect() runs.
---
Nitpick comments:
In `@openevsehttp/override.py`:
- Around line 147-155: The code assumes response is a dict when calling
self._evse.process_request and using response.get(...), but later checks
isinstance(response, dict) for msg extraction; refactor by first checking if
response is a dict right after response = await self._evse.process_request(...)
(use isinstance(response, dict)), handle the non-dict case consistently (e.g.,
log the raw response via _LOGGER.error or set msg = response and raise
UnknownError as appropriate), and then safely call response.get("ok", True) and
response.get("msg") for the dict path so the subsequent msg =
response.get("msg") logic (and the UnknownError raise) no longer relies on a
redundant isinstance check. Ensure references to _evse.process_request,
response, _LOGGER, and UnknownError are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce5239c8-fd6f-470f-b4b4-91b184e9921f
📒 Files selected for processing (13)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (5)
- tests/fixtures/v_legacy_json/status.json
- openevsehttp/const.py
- tests/test_firmware.py
- tests/fixtures/v_legacy_json/config.json
- tests/test_override.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_external_session.py
…sk storage to a set
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openevsehttp/client.py (1)
213-226: Using asetfor tasks prevents duplicate detection.Lines 213-216 and 223-226 store tasks in a
setliteral. Sinceasyncio.Taskobjects are not hashable by default in a meaningful way (they use object identity), this works but prevents any useful duplicate detection. More importantly, when checkingactive_tasksat Line 209, iterating over a set is fine, but the original intent of using a list made iteration order predictable.This is minor since the tasks are immediately created and the set behavior is acceptable here, but consider whether a list was intended for consistency with the
Noneassignment at Line 319.Optional: Use list for consistency
if not self._ws_listening and not active_tasks: _LOGGER.debug("Setting up websocket ping...") - self.tasks = { + self.tasks = [ self._loop.create_task(self.websocket.listen()), self._loop.create_task(self.repeat(300, self.websocket.keepalive)), - } + ] elif active_tasks: _LOGGER.debug("Cleaning up orphaned websocket tasks before restart...") for task in active_tasks: if not task.done(): task.cancel() await asyncio.gather(*active_tasks, return_exceptions=True) - self.tasks = { + self.tasks = [ self._loop.create_task(self.websocket.listen()), self._loop.create_task(self.repeat(300, self.websocket.keepalive)), - } + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 213 - 226, The current code assigns self.tasks using a set literal of asyncio Task objects (created via self._loop.create_task for websocket.listen and repeat) which can hide ordering and duplicate detection; change self.tasks to be a list instead of a set in both places where it's assigned (the two blocks creating tasks) so it uses predictable order and is consistent with other code that treats tasks as a sequence (e.g., active_tasks handling and the None assignment later). Locate the assignments that create tasks (references: self.tasks, self._loop.create_task(self.websocket.listen), self._loop.create_task(self.repeat(300, self.websocket.keepalive))) and replace the set literal with a list literal to fix the issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openevsehttp/client.py`:
- Around line 213-226: The current code assigns self.tasks using a set literal
of asyncio Task objects (created via self._loop.create_task for websocket.listen
and repeat) which can hide ordering and duplicate detection; change self.tasks
to be a list instead of a set in both places where it's assigned (the two blocks
creating tasks) so it uses predictable order and is consistent with other code
that treats tasks as a sequence (e.g., active_tasks handling and the None
assignment later). Locate the assignments that create tasks (references:
self.tasks, self._loop.create_task(self.websocket.listen),
self._loop.create_task(self.repeat(300, self.websocket.keepalive))) and replace
the set literal with a list literal to fix the issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 065fd40b-43f4-463e-90a3-a6483345d70b
📒 Files selected for processing (13)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/v_legacy_json/status.json
- tests/test_firmware.py
- tests/fixtures/v_legacy_json/config.json
🚧 Files skipped from review as they are similar to previous changes (4)
- openevsehttp/const.py
- openevsehttp/override.py
- tests/test_client.py
- tests/test_requester.py
There was a problem hiding this comment.
♻️ Duplicate comments (5)
openevsehttp/client.py (2)
115-123:⚠️ Potential issue | 🟠 MajorDon't let websocket mode skip
/statuswhen the cache is empty.Line 121 optimizes
update()down to/configonly whenever_ws_listeningis true. The legacy branch inopenevsehttp/override.py:83-138callsawait self._evse.update()specifically to recover_status["state"]; with an empty_status, this still returns without/statusandtoggle_override()falls intoUnknownError.Suggested fix
- if not self._ws_listening or force_full: + if force_full or not self._ws_listening or "state" not in self._status: urls = [f"{self.url}status", f"{self.url}config"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 115 - 123, The update() method currently skips fetching /status when _ws_listening is true, which breaks callers (like override.toggle_override) that rely on _status being populated; modify async def update(self, force_full: bool = False) in client.py so it still includes f"{self.url}status" whenever the local cache _status is empty or missing the expected "state" key (in addition to when force_full is True or _ws_listening is False), ensuring callers can recover _status["state"] after awaiting self._evse.update().
671-691:⚠️ Potential issue | 🟠 Major
set_divert_mode()returns before the cached mode changes.This path succeeds with
"Divert Mode changed", but that message is not one of theRequestersuccess markers that triggersfull_refresh(). Since this method also leaves_statusuntouched, a successful call can still leavedivertmodereporting the previous mode for polling-only clients.Possible targeted fix
result = self._extract_msg(response) if result != "Divert Mode changed": _LOGGER.error("Problem issuing command. Response: %s", response) raise UnknownError + self._status["divertmode"] = new_mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 671 - 691, set_divert_mode currently treats a successful "Divert Mode changed" response as final but neither calls full_refresh nor updates the cached _status/_status["divertmode"], so polling clients may still report the old mode; after verifying result == "Divert Mode changed" (and after the existing UnknownError checks), either call the Requester.full_refresh() method (or await self.full_refresh() if async on this class) to refresh the device state or update the cached state directly (e.g., set self._status["divertmode"] = new_mode or its string equivalent) so the cached value reflects the change; locate symbols set_divert_mode, _extract_msg, process_request, full_refresh, and _status in this file to implement the fix.tests/test_override.py (1)
122-133:⚠️ Potential issue | 🟡 MinorThese success-path tests still hide post-write refresh regressions.
After the initial
update(), the fixture's one-shot/statusand/configmocks are already consumed. The subsequent successful RAPI/override writes triggerRequester.full_refresh(), but missing follow-up GETs are swallowed there, so these cases can stay green even if the refresh path breaks. Either add repeat GET stubs or disablerequester._update_callbackin tests that only care about the write request.Also applies to: 316-344, 437-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_override.py` around lines 122 - 133, The test test_toggle_override_v2 (and similar tests) hides post-write refresh regressions because after test_charger_legacy.update() the one-shot /status and /config mocks are consumed, so the subsequent toggle_override() triggers Requester.full_refresh() which expects follow-up GETs; fix by either adding repeat GET stubs for the follow-up /status and /config calls after the write stub so the refresh succeeds, or disable the refresh callback by setting test_charger_legacy.requester._update_callback = None (or equivalent) before calling await test_charger_legacy.toggle_override(), and apply the same change to the other affected tests referenced (lines ~316-344, 437-470).tests/test_client.py (1)
1433-1450:⚠️ Potential issue | 🟡 MinorThis test currently asserts the stale-cache fallback, not the successful write behavior.
OpenEVSEwiresRequester.full_refresh()by default, so the successfulset_led_brightness()at Line 1448 will attempt a follow-up/status+/configrefresh. Since those GETs are not stubbed here, the refresh fails inside Requester’s swallowed-exception branch andcharger.led_brightnessstays128for the wrong reason. Disable the callback if this is meant to be a pure request test, or stub the refresh and assert the updated value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 1433 - 1450, The test test_led_brightness currently asserts a stale-cache fallback because OpenEVSE wires Requester.full_refresh() by default; either disable that refresh callback before calling set_led_brightness (so the test only verifies the POST) or stub the follow-up GETs (/status and /config) that Requester.full_refresh triggers so the refresh succeeds and led_brightness reflects the written value; locate the test function test_led_brightness and modify it to call the appropriate API on the Requester/OpenEVSE to disable refresh or add mocked responses for the GETs before awaiting set_led_brightness and asserting charger.led_brightness.openevsehttp/requester.py (1)
138-149:⚠️ Potential issue | 🟠 MajorInclude successful
DELETEwrites in the coalesced refresh.Line 139 still excludes
delete.openevsehttp/override.py:139-160clears overrides withmethod="delete"and does not patch any local cache, so a successful clear can return while cached status/config still reflect the old state.Targeted fix
- method in ("post", "patch", "put") + method in ("post", "patch", "put", "delete")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 138 - 149, The current conditional in requester.py that decides to trigger coalesced refresh excludes "delete" and therefore misses successful DELETE writes (which override.py uses when clearing overrides), so update the conditional in the block that checks method in ("post", "patch", "put") to also include "delete" (i.e., method in ("post", "patch", "put", "delete")) so that when a DELETE returns a successful message (matching the existing UPDATE_TRIGGERS/_update_callback/msg/ret/cmd checks) the coalesced refresh logic runs and local cache/status gets refreshed; keep the rest of the checks (isinstance(message, dict), message.get("ok") etc.) unchanged and ensure the condition still references UPDATE_TRIGGERS and self._update_callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openevsehttp/client.py`:
- Around line 115-123: The update() method currently skips fetching /status when
_ws_listening is true, which breaks callers (like override.toggle_override) that
rely on _status being populated; modify async def update(self, force_full: bool
= False) in client.py so it still includes f"{self.url}status" whenever the
local cache _status is empty or missing the expected "state" key (in addition to
when force_full is True or _ws_listening is False), ensuring callers can recover
_status["state"] after awaiting self._evse.update().
- Around line 671-691: set_divert_mode currently treats a successful "Divert
Mode changed" response as final but neither calls full_refresh nor updates the
cached _status/_status["divertmode"], so polling clients may still report the
old mode; after verifying result == "Divert Mode changed" (and after the
existing UnknownError checks), either call the Requester.full_refresh() method
(or await self.full_refresh() if async on this class) to refresh the device
state or update the cached state directly (e.g., set self._status["divertmode"]
= new_mode or its string equivalent) so the cached value reflects the change;
locate symbols set_divert_mode, _extract_msg, process_request, full_refresh, and
_status in this file to implement the fix.
In `@openevsehttp/requester.py`:
- Around line 138-149: The current conditional in requester.py that decides to
trigger coalesced refresh excludes "delete" and therefore misses successful
DELETE writes (which override.py uses when clearing overrides), so update the
conditional in the block that checks method in ("post", "patch", "put") to also
include "delete" (i.e., method in ("post", "patch", "put", "delete")) so that
when a DELETE returns a successful message (matching the existing
UPDATE_TRIGGERS/_update_callback/msg/ret/cmd checks) the coalesced refresh logic
runs and local cache/status gets refreshed; keep the rest of the checks
(isinstance(message, dict), message.get("ok") etc.) unchanged and ensure the
condition still references UPDATE_TRIGGERS and self._update_callback.
In `@tests/test_client.py`:
- Around line 1433-1450: The test test_led_brightness currently asserts a
stale-cache fallback because OpenEVSE wires Requester.full_refresh() by default;
either disable that refresh callback before calling set_led_brightness (so the
test only verifies the POST) or stub the follow-up GETs (/status and /config)
that Requester.full_refresh triggers so the refresh succeeds and led_brightness
reflects the written value; locate the test function test_led_brightness and
modify it to call the appropriate API on the Requester/OpenEVSE to disable
refresh or add mocked responses for the GETs before awaiting set_led_brightness
and asserting charger.led_brightness.
In `@tests/test_override.py`:
- Around line 122-133: The test test_toggle_override_v2 (and similar tests)
hides post-write refresh regressions because after test_charger_legacy.update()
the one-shot /status and /config mocks are consumed, so the subsequent
toggle_override() triggers Requester.full_refresh() which expects follow-up
GETs; fix by either adding repeat GET stubs for the follow-up /status and
/config calls after the write stub so the refresh succeeds, or disable the
refresh callback by setting test_charger_legacy.requester._update_callback =
None (or equivalent) before calling await test_charger_legacy.toggle_override(),
and apply the same change to the other affected tests referenced (lines
~316-344, 437-470).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d787327-7f2b-4d69-9aa1-fe560c36ceea
📒 Files selected for processing (13)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (4)
- tests/fixtures/v_legacy_json/status.json
- tests/fixtures/v_legacy_json/config.json
- tests/test_firmware.py
- tests/test_requester.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_external_session.py
- openevsehttp/const.py
- openevsehttp/override.py
…n update callbacks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/test_client.py (2)
2222-2224:⚠️ Potential issue | 🟡 MinorKeep
charger.tasksas a set in these websocket task tests.
OpenEVSEstorestasksas a set. Switching to a list here exercises a different container contract and can miss regressions around the lifecycle code’s set operations.💡 Suggested fix
- charger.tasks = [task] + charger.tasks = {task}Also applies to: 2297-2298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 2222 - 2224, The test incorrectly assigns charger.tasks as a list; keep it as the same container type used in OpenEVSE by making charger.tasks a set (e.g., use {task} or set([task])) so set-specific lifecycle behavior is exercised; update both occurrences around the dummy()/ws_disconnect() calls (the assignment near the task = asyncio.create_task(dummy()) before await charger.ws_disconnect() and the similar assignment at the other location) so the tests use a set instead of a list.
606-629:⚠️ Potential issue | 🟡 MinorThese happy-path mutator tests still hide the post-write refresh leg.
After the initial
await test_charger.update(), thetests/conftest.pytest_chargerfixture has already consumed its one-shot/statusand/configmocks. A successful{"msg": "OK"}or{"msg": "done"}here triggersRequester._update_callback(), the refresh then fails insideRequester.process_request(), and that exception is swallowed, so these tests can stay green while the write-refresh path is broken. Either disable the callback in these cases or make the GET mocks repeatable before asserting the write path.💡 Example fix
async def test_set_current(test_charger, mock_aioclient, caplog): """Verify that set_current correctly updates the current limit via the override API.""" await test_charger.update() + test_charger.requester.set_update_callback(None)Also applies to: 1230-1241, 1295-1306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_client.py` around lines 606 - 629, The test's happy-path for test_set_current relies on a post-write callback that triggers a follow-up GET, but the test_charger fixture already consumed the one-shot /status and /config mocks so that refresh fails silently; update the test to either disable the Requester._update_callback() for the POST response or make the GET mocks repeatable before performing the write. Concretely, in test_set_current adjust the mock_aioclient setup so the subsequent GET to "http://openevse.test.tld/override" (or the follow-up /status and /config used by Requester.process_request()) is registered as repeatable (or registered again) so the refresh performed by Requester._update_callback()/Requester.process_request() succeeds, or alternatively stub/disable the update callback for that POST response to avoid invoking the refresh path. Ensure changes reference test_set_current, test_charger fixture, Requester._update_callback, and Requester.process_request to locate the relevant logic.tests/test_override.py (1)
391-404:⚠️ Potential issue | 🟡 MinorThis happy-path clear test still swallows the refresh callback.
clear_override()returns{"msg": "OK"}, whichRequester.process_request()treats as a successful delete and immediately refreshes. Becauseawait test_charger.update()already consumed the base fixture’s one-shot/statusand/configmocks, that refresh fails inside the swallowed callback path, so this assertion can stay green while post-delete cache invalidation is broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_override.py` around lines 391 - 404, The test is swallowing the post-delete refresh callback because clear_override() triggers an immediate refresh via Requester.process_request() but the one-shot /status and /config mocks were already consumed by await test_charger.update(); fix by arranging mocks for the expected refresh before calling clear_override(): add mock_aioclient.get(...) responses for the charger’s status and config endpoints (the equivalents used by Requester.process_request()) or otherwise stub Requester.process_request() to return a successful refresh, then call await test_charger.clear_override() and keep the existing assertion about "Clear response: OK" so the refresh path is exercised rather than left to fail silently.openevsehttp/requester.py (1)
143-147:⚠️ Potential issue | 🟠 MajorDon’t refresh the cache for failed legacy RAPI replies.
Legacy callers only treat replies with
ret.startswith("$OK")as success. This branch currently refreshes on any payload that merely containscmd, so a failed{"cmd": "...", "ret": "$NK..."}can still run_update_callback()and make the cache look freshly updated before the caller raises or returns failure.💡 Suggested fix
and ( any(key in message for key in UPDATE_TRIGGERS) or message.get("msg") in ("done", "no change", "OK") - or message.get("ret") == "$OK" - or message.get("cmd") is not None + or str(message.get("ret", "")).startswith("$OK") ) ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/requester.py` around lines 143 - 147, The cache refresh condition currently treats any message with a "cmd" field as success and calls _update_callback, which lets failed legacy RAPI replies like {"cmd": "...", "ret": "$NK..."} refresh the cache; change the boolean logic in the branch that uses UPDATE_TRIGGERS so that when message.get("cmd") is present you only treat it as successful if message.get("ret", "").startswith("$OK") (i.e., require ret to begin with "$OK" for legacy callers) — update the compound condition around UPDATE_TRIGGERS and message.get("cmd") accordingly to prevent _update_callback from running on failed ret values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/override.py`:
- Around line 49-52: The current check in the override retrieval accepts
payloads that only contain "state", which allows partial reads to be used as a
merge base in set() and produces malformed POSTs; update the validation in the
method that calls await self.get() (the override getter used by set()) to
require that data is a dict with data.get("ok") not False and that it contains
all expected fields ("state", "charge_current", "max_current", "energy_limit",
"time_limit", "auto_release") and raise UnknownError if any are missing so set()
never receives a partial override object.
---
Duplicate comments:
In `@openevsehttp/requester.py`:
- Around line 143-147: The cache refresh condition currently treats any message
with a "cmd" field as success and calls _update_callback, which lets failed
legacy RAPI replies like {"cmd": "...", "ret": "$NK..."} refresh the cache;
change the boolean logic in the branch that uses UPDATE_TRIGGERS so that when
message.get("cmd") is present you only treat it as successful if
message.get("ret", "").startswith("$OK") (i.e., require ret to begin with "$OK"
for legacy callers) — update the compound condition around UPDATE_TRIGGERS and
message.get("cmd") accordingly to prevent _update_callback from running on
failed ret values.
In `@tests/test_client.py`:
- Around line 2222-2224: The test incorrectly assigns charger.tasks as a list;
keep it as the same container type used in OpenEVSE by making charger.tasks a
set (e.g., use {task} or set([task])) so set-specific lifecycle behavior is
exercised; update both occurrences around the dummy()/ws_disconnect() calls (the
assignment near the task = asyncio.create_task(dummy()) before await
charger.ws_disconnect() and the similar assignment at the other location) so the
tests use a set instead of a list.
- Around line 606-629: The test's happy-path for test_set_current relies on a
post-write callback that triggers a follow-up GET, but the test_charger fixture
already consumed the one-shot /status and /config mocks so that refresh fails
silently; update the test to either disable the Requester._update_callback() for
the POST response or make the GET mocks repeatable before performing the write.
Concretely, in test_set_current adjust the mock_aioclient setup so the
subsequent GET to "http://openevse.test.tld/override" (or the follow-up /status
and /config used by Requester.process_request()) is registered as repeatable (or
registered again) so the refresh performed by
Requester._update_callback()/Requester.process_request() succeeds, or
alternatively stub/disable the update callback for that POST response to avoid
invoking the refresh path. Ensure changes reference test_set_current,
test_charger fixture, Requester._update_callback, and Requester.process_request
to locate the relevant logic.
In `@tests/test_override.py`:
- Around line 391-404: The test is swallowing the post-delete refresh callback
because clear_override() triggers an immediate refresh via
Requester.process_request() but the one-shot /status and /config mocks were
already consumed by await test_charger.update(); fix by arranging mocks for the
expected refresh before calling clear_override(): add mock_aioclient.get(...)
responses for the charger’s status and config endpoints (the equivalents used by
Requester.process_request()) or otherwise stub Requester.process_request() to
return a successful refresh, then call await test_charger.clear_override() and
keep the existing assertion about "Clear response: OK" so the refresh path is
exercised rather than left to fail silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95fcacfe-f92c-41e9-ac10-ae2e1b560c95
📒 Files selected for processing (13)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (5)
- tests/fixtures/v_legacy_json/status.json
- tests/fixtures/v_legacy_json/config.json
- openevsehttp/const.py
- tests/test_firmware.py
- tests/test_requester.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_external_session.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_websocket.py (1)
331-377: Consider splittingtest_websocket_coverage_gapsinto two tests for isolation.It currently validates STOPPED-loop break and pong timestamp behavior in one function; separating them would make failures more pinpointed and easier to diagnose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_websocket.py` around lines 331 - 377, Split the monolithic test_websocket_coverage_gaps into two isolated tests: one (e.g., test_websocket_breaks_on_stopped) that only verifies the loop breaks when ws_client.state becomes STATE_STOPPED by using the async_iter_stop generator, a patched aiohttp.ClientSession.ws_connect returning mock_ws, and a patched asyncio.sleep; and a second (e.g., test_websocket_handles_pong_timestamp) that only verifies pong handling by using async_iter_pong, setting ws_client.state to STATE_CONNECTED before running ws_client.running(), and asserting ws_client._pong is a datetime after completion; ensure each new test resets ws_client.state, sets mock_ws.__aiter__.side_effect appropriately, and scopes patches per-test so mocks and state do not leak between tests.openevsehttp/client.py (1)
270-274: Keep websocket refresh triggers aligned withRequester.
Requester.process_request()already treats success envelopes like{"msg": "done"},"OK", and"no change"as refresh signals, but this websocket branch only reacts to version keys or RAPI-shaped payloads. Matching the same success markers here would avoid transport-specific stale-cache behavior.Possible alignment
if ( any(key in keys for key in UPDATE_TRIGGERS) + or data.get("msg") in ("done", "no change", "OK") or data.get("ret") == "$OK" or "cmd" in keys ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/client.py` around lines 270 - 274, The websocket branch that decides to refresh only checks UPDATE_TRIGGERS, RAPI-shaped payloads, or data.get("ret") == "$OK"; update that conditional to also treat the same success envelopes used by Requester.process_request as refresh signals — specifically add checks for data == "OK", data == "no change", and data.get("msg") == "done" (so the conditional in the block containing UPDATE_TRIGGERS will trigger on these values), ensuring behavior matches Requester.process_request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 186-199: The code reuses an existing OpenEVSEWebsocket instance in
_start_listening, which prevents reconnection when the previous instance has
state STATE_STOPPED; update _start_listening (or ws_start caller) to detect a
stopped websocket (e.g., self.websocket.state == "stopped" or STATE_STOPPED) and
recreate the OpenEVSEWebsocket object (or set self.websocket = None so the
creation block runs) before calling listen; reference OpenEVSEWebsocket.listen,
the websocket attribute, and _start_listening (and ws_disconnect) to ensure
stopped instances are replaced rather than reused.
In `@openevsehttp/requester.py`:
- Around line 62-63: The current truthy check drops authentication when password
is an empty string; change the conditional in requester.py so it checks for
None, not truthiness. For example, replace the if self._user and self._pwd:
guard with an explicit presence check such as if self._user is not None: then
construct auth = aiohttp.BasicAuth(self._user, self._pwd) so that an
intentionally blank password (self._pwd == "") still sends Authorization;
reference the variables self._user, self._pwd and the auth assignment to locate
the change.
In `@tests/test_override.py`:
- Around line 568-583: The test never exercises the POST failure because the GET
stub for "/override" returns only {"state":"disabled"}, triggering
Override.set()'s required-key validation and aborting before the POST; change
the GET stub used before calling charger.set_override(...) so it returns a
complete override payload that satisfies Override.set()'s required keys (i.e.
include the fields Override.set() expects so the flow proceeds to the POST),
then keep the mock_aioclient.post(...) that returns {"msg":"error","ok":false}
and assert that await charger.set_override(charge_current=16) raises
UnknownError; reference OpenEVSE, charger.update(), Override.set(),
set_override, and mock_aioclient.post in your changes.
---
Nitpick comments:
In `@openevsehttp/client.py`:
- Around line 270-274: The websocket branch that decides to refresh only checks
UPDATE_TRIGGERS, RAPI-shaped payloads, or data.get("ret") == "$OK"; update that
conditional to also treat the same success envelopes used by
Requester.process_request as refresh signals — specifically add checks for data
== "OK", data == "no change", and data.get("msg") == "done" (so the conditional
in the block containing UPDATE_TRIGGERS will trigger on these values), ensuring
behavior matches Requester.process_request.
In `@tests/test_websocket.py`:
- Around line 331-377: Split the monolithic test_websocket_coverage_gaps into
two isolated tests: one (e.g., test_websocket_breaks_on_stopped) that only
verifies the loop breaks when ws_client.state becomes STATE_STOPPED by using the
async_iter_stop generator, a patched aiohttp.ClientSession.ws_connect returning
mock_ws, and a patched asyncio.sleep; and a second (e.g.,
test_websocket_handles_pong_timestamp) that only verifies pong handling by using
async_iter_pong, setting ws_client.state to STATE_CONNECTED before running
ws_client.running(), and asserting ws_client._pong is a datetime after
completion; ensure each new test resets ws_client.state, sets
mock_ws.__aiter__.side_effect appropriately, and scopes patches per-test so
mocks and state do not leak between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e73848c4-2d05-483f-8485-60c3676402e2
📒 Files selected for processing (13)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (5)
- tests/fixtures/v_legacy_json/status.json
- tests/fixtures/v_legacy_json/config.json
- openevsehttp/const.py
- tests/test_firmware.py
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_external_session.py
…management for proper reconnection
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_websocket.py (1)
122-128: Minor: Unused unpacked variable.The
argsvariable frommock_connect.call_argsis unpacked but never used. Consider using_to indicate it's intentionally ignored.Suggested fix
- args, kwargs = mock_connect.call_args + _, kwargs = mock_connect.call_args auth = kwargs.get("auth")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_websocket.py` around lines 122 - 128, The test unpacks mock_connect.call_args into args and kwargs but never uses args; change the unpack to ignore the positional args (e.g., use "_" or "_args") so intent is clear — replace the line "args, kwargs = mock_connect.call_args" with an unused-variable unpack like "_ , kwargs = mock_connect.call_args" and leave the subsequent assertions on kwargs/auth unchanged (references: mock_connect.call_args and auth).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_websocket.py`:
- Around line 122-128: The test unpacks mock_connect.call_args into args and
kwargs but never uses args; change the unpack to ignore the positional args
(e.g., use "_" or "_args") so intent is clear — replace the line "args, kwargs =
mock_connect.call_args" with an unused-variable unpack like "_ , kwargs =
mock_connect.call_args" and leave the subsequent assertions on kwargs/auth
unchanged (references: mock_connect.call_args and auth).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa4f14b8-3ee1-4669-8348-8e767ccb156d
📒 Files selected for processing (14)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pyopenevsehttp/websocket.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (5)
- tests/fixtures/v_legacy_json/status.json
- tests/fixtures/v_legacy_json/config.json
- openevsehttp/const.py
- tests/test_firmware.py
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_external_session.py
- openevsehttp/override.py
- tests/test_requester.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
106-127:⚠️ Potential issue | 🟡 MinorMake the shared write-path fixtures repeatable too.
Successful writes now trigger a best-effort
full_refresh(). These two fixtures are safe, but the common fixtures used by most mutator tests still expose one-shot/statusand/configmocks, so after the initialupdate()the follow-up refresh misses those mocks and the exception gets swallowed insideRequester. That weakens the write-path tests because they can pass without actually exercising the refresh path.Also applies to: 210-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 106 - 127, The mocks for /status, /config and /ws in the test fixtures are one-shot which breaks write-path tests because successful writes trigger a best-effort full_refresh() that needs those endpoints again; change the mock_aioclient.get calls for TEST_URL_STATUS, TEST_URL_CONFIG and TEST_URL_WS in the shared fixtures (e.g., the test_charger_new fixture that returns main.OpenEVSE(TEST_TLD)) to pass repeat=True so the endpoints are repeatable, and apply the same change to the other shared fixtures used by mutator tests (the other fixture blocks that register TEST_URL_STATUS/TEST_URL_CONFIG/TEST_URL_WS) so refresh attempts won't miss the mocks and errors won’t be swallowed in Requester.
♻️ Duplicate comments (1)
openevsehttp/websocket.py (1)
61-67:⚠️ Potential issue | 🟠 MajorThe thread-safe fallback still breaks in the case it is trying to recover from.
If
asyncio.create_task()raises here from a worker thread, Line 66 can raise the sameRuntimeError, so the state callback is never scheduled. Capture the owning loop once and schedule against that stored loop instead of resolving a loop inside the fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openevsehttp/websocket.py` around lines 61 - 67, The thread-safe fallback can still raise RuntimeError because it calls asyncio.create_task() inside call_soon_threadsafe; instead capture the event loop once (e.g. call loop = asyncio.get_event_loop() or use self._loop if available) before attempting asyncio.create_task, and in the except branch schedule the coroutine on that captured loop with loop.call_soon_threadsafe(loop.create_task, coro) (or loop.run_coroutine_threadsafe(coro, loop) if preferred) so the callback created for SIGNAL_CONNECTION_STATE via self.callback(...) is reliably scheduled from worker threads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 285-292: The user callback can be re-entered because websocket and
post-write refresh paths call self.callback directly; serialize callback
execution by adding a single invoker that both paths use: implement an async
helper on Requester (e.g., _invoke_callback or _run_callback) that uses an
asyncio.Lock or an internal Task tracker (e.g., self._callback_lock or
self._callback_task) to ensure only one invocation runs at a time, awaits any
currently running invocation before running the next, handles sync vs coroutine
callbacks the same way as the current logic (detect with is_coroutine_function
and await if needed), and preserves the existing exception logging; replace
direct calls to self.callback() / await self.callback() in the full_refresh(),
websocket handler and post-write refresh blocks with a call to this new invoker
so callbacks cannot overlap.
- Around line 146-148: The code currently assigns response to self._config and
logs the entire dict (_LOGGER.debug("Config update: %s", self._config)), which
can leak secrets; change this to store the full response in self._config but do
not log it raw—replace the debug call with either logging only the config keys
or a redacted version that masks known sensitive fields (e.g., WIFI_PASSWORD,
UI_PASSWORD, MQTT_PASSWORD, API_KEY); implement a small helper (e.g.,
_redact_config or SENSITIVE_FIELDS constant) and call that from the same place
where self._config is set to produce a safe debug message listing keys or masked
values instead of the full payload.
In `@openevsehttp/websocket.py`:
- Around line 87-88: When building aiohttp.BasicAuth in the websocket code path
that checks self._user, coerce self._password from None to the empty string
before passing it to BasicAuth to avoid ValueError; update the block that
currently does "if self._user: auth = aiohttp.BasicAuth(self._user,
self._password)" to use a password value like (self._password or "") or an
explicit conditional so BasicAuth never receives None (refer to the symbols
self._user, self._password and the BasicAuth construction).
---
Outside diff comments:
In `@tests/conftest.py`:
- Around line 106-127: The mocks for /status, /config and /ws in the test
fixtures are one-shot which breaks write-path tests because successful writes
trigger a best-effort full_refresh() that needs those endpoints again; change
the mock_aioclient.get calls for TEST_URL_STATUS, TEST_URL_CONFIG and
TEST_URL_WS in the shared fixtures (e.g., the test_charger_new fixture that
returns main.OpenEVSE(TEST_TLD)) to pass repeat=True so the endpoints are
repeatable, and apply the same change to the other shared fixtures used by
mutator tests (the other fixture blocks that register
TEST_URL_STATUS/TEST_URL_CONFIG/TEST_URL_WS) so refresh attempts won't miss the
mocks and errors won’t be swallowed in Requester.
---
Duplicate comments:
In `@openevsehttp/websocket.py`:
- Around line 61-67: The thread-safe fallback can still raise RuntimeError
because it calls asyncio.create_task() inside call_soon_threadsafe; instead
capture the event loop once (e.g. call loop = asyncio.get_event_loop() or use
self._loop if available) before attempting asyncio.create_task, and in the
except branch schedule the coroutine on that captured loop with
loop.call_soon_threadsafe(loop.create_task, coro) (or
loop.run_coroutine_threadsafe(coro, loop) if preferred) so the callback created
for SIGNAL_CONNECTION_STATE via self.callback(...) is reliably scheduled from
worker threads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad78bcc6-c35f-4e55-b7ba-2bc3ad933405
📒 Files selected for processing (14)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pyopenevsehttp/websocket.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (4)
- tests/fixtures/v_legacy_json/config.json
- openevsehttp/const.py
- tests/fixtures/v_legacy_json/status.json
- tests/test_override.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_external_session.py
- tests/test_requester.py
- tests/test_firmware.py
- openevsehttp/override.py
… empty strings in websocket authentication
…ence and using loop-specific task creation
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openevsehttp/client.py`:
- Around line 87-88: _invoke_callback currently holds _callback_lock for the
entire user callback which deadlocks reentrant writes (e.g.
Requester.full_refresh calling back into _invoke_callback). Change the logic to
coalesce nested notifications instead of awaiting a plain lock: use a short-held
lock only to check/set a _callback_pending (or increment a counter) and to
ensure a single "notification task" is scheduled; release the lock before
invoking user callbacks so they run without holding _callback_lock, and
loop/swap the pending flag so if additional notifications arrive while callbacks
run you re-run callbacks once more (same pattern used by
Requester.full_refresh). Apply the same change to the other invocation site (the
block around lines 292-309) so both paths coalesce and do not await while user
callbacks run.
- Around line 251-259: The branch handling STATE_STOPPED should clear
self._ws_listening unconditionally so the flag isn't left True when the socket
stops without an error; modify the branch in the websocket state handler where
it checks for data == STATE_STOPPED (currently only clearing self._ws_listening
when error is truthy) to always set self._ws_listening = False and adjust the
log call accordingly (keep the debug log for errors but still clear the flag on
non-error stops) so update() will no longer skip /status after a stopped
websocket.
In `@openevsehttp/requester.py`:
- Around line 29-35: The Requester currently allows pwd=None which later gets
passed directly to aiohttp.BasicAuth and raises ValueError; update
Requester.__init__ (and the code path that constructs aiohttp.BasicAuth, e.g.,
where self._pwd is used around the BasicAuth creation) to normalize None to an
empty string (self._pwd = pwd or "") so that callers passing None behave the
same as the websocket transport and no None is forwarded to aiohttp.BasicAuth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ad5eb1f-dbde-40a8-83ce-7a38241f8a57
📒 Files selected for processing (14)
openevsehttp/client.pyopenevsehttp/const.pyopenevsehttp/override.pyopenevsehttp/requester.pyopenevsehttp/websocket.pytests/conftest.pytests/fixtures/v_legacy_json/config.jsontests/fixtures/v_legacy_json/status.jsontests/test_client.pytests/test_external_session.pytests/test_firmware.pytests/test_override.pytests/test_requester.pytests/test_websocket.py
✅ Files skipped from review due to trivial changes (5)
- tests/fixtures/v_legacy_json/config.json
- tests/test_external_session.py
- tests/fixtures/v_legacy_json/status.json
- tests/test_override.py
- tests/test_client.py
🚧 Files skipped from review as they are similar to previous changes (5)
- openevsehttp/const.py
- openevsehttp/websocket.py
- openevsehttp/override.py
- tests/test_firmware.py
- tests/test_requester.py
Comment on lines
+87
to
+88
| self._callback_lock = asyncio.Lock() | ||
| self._session = session |
There was a problem hiding this comment.
The serialized callback path can deadlock reentrant writes.
_invoke_callback() holds _callback_lock for the full duration of the user callback. If an async callback issues a mutating API call, Requester routes the success path through full_refresh(), and full_refresh() immediately awaits _invoke_callback() again. That second wait is on the same lock, so the write can hang until the outer callback returns.
Please coalesce nested callback notifications here the same way Requester coalesces nested refreshes, instead of awaiting a plain lock.
Also applies to: 292-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openevsehttp/client.py` around lines 87 - 88, _invoke_callback currently
holds _callback_lock for the entire user callback which deadlocks reentrant
writes (e.g. Requester.full_refresh calling back into _invoke_callback). Change
the logic to coalesce nested notifications instead of awaiting a plain lock: use
a short-held lock only to check/set a _callback_pending (or increment a counter)
and to ensure a single "notification task" is scheduled; release the lock before
invoking user callbacks so they run without holding _callback_lock, and
loop/swap the pending flag so if additional notifications arrive while callbacks
run you re-run callbacks once more (same pattern used by
Requester.full_refresh). Apply the same change to the other invocation site (the
block around lines 292-309) so both paths coalesce and do not await while user
callbacks run.
Comment on lines
+251
to
+259
| # Stopped websockets without errors are expected during shutdown | ||
| # and ignored | ||
| elif data == STATE_STOPPED and error: | ||
| _LOGGER.debug( | ||
| "Websocket to %s failed, aborting [Error: %s]", | ||
| uri, | ||
| error, | ||
| ) | ||
| self._ws_listening = False |
There was a problem hiding this comment.
Clear _ws_listening on every STATE_STOPPED, not just error stops.
If the socket reaches STATE_STOPPED without a preceding STATE_DISCONNECTED, this branch leaves _ws_listening stuck True. The next update() will then keep skipping /status even though the websocket is already gone.
♻️ Possible fix
- elif data == STATE_STOPPED and error:
- _LOGGER.debug(
- "Websocket to %s failed, aborting [Error: %s]",
- uri,
- error,
- )
- self._ws_listening = False
+ elif data == STATE_STOPPED:
+ if error:
+ _LOGGER.debug(
+ "Websocket to %s failed, aborting [Error: %s]",
+ uri,
+ error,
+ )
+ self._ws_listening = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openevsehttp/client.py` around lines 251 - 259, The branch handling
STATE_STOPPED should clear self._ws_listening unconditionally so the flag isn't
left True when the socket stops without an error; modify the branch in the
websocket state handler where it checks for data == STATE_STOPPED (currently
only clearing self._ws_listening when error is truthy) to always set
self._ws_listening = False and adjust the log call accordingly (keep the debug
log for errors but still clear the flag on non-error stops) so update() will no
longer skip /status after a stopped websocket.
Comment on lines
+29
to
+35
| user: str = "", | ||
| pwd: str = "", | ||
| session: aiohttp.ClientSession | None = None, | ||
| ) -> None: | ||
| """Initialize the requester.""" | ||
| self._user = user | ||
| self._pwd = pwd |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does aiohttp.BasicAuth treat password=None as an empty password, or does it require the password argument to be a string?
💡 Result:
aiohttp.BasicAuth does not treat password=None as an empty password. It explicitly raises ValueError("None is not allowed as password value") if password is None. The password argument must be a string; the default is the empty string "", which is treated as an empty password.
Citations:
- 1: https://aiohttp.readthedocs.org/en/stable/_modules/aiohttp/helpers.html
- 2: http://aiohttp-kxepal-test.readthedocs.org/en/latest/_modules/aiohttp/helpers.html
- 3: Enhanced basic auth decode aio-libs/aiohttp#3239
- 4: https://github.com/aio-libs/aiohttp
🏁 Script executed:
cat -n openevsehttp/requester.py | sed -n '1,80p'Repository: firstof9/python-openevse-http
Length of output: 3034
🏁 Script executed:
rg "BasicAuth|aiohttp.BasicAuth" openevsehttp/requester.py -A 3 -B 3Repository: firstof9/python-openevse-http
Length of output: 269
🏁 Script executed:
rg "self._pwd" openevsehttp/requester.py -A 2 -B 2Repository: firstof9/python-openevse-http
Length of output: 369
🏁 Script executed:
fd "test_websocket" --type f -exec grep -l "password.*None\|pwd.*None" {} \;Repository: firstof9/python-openevse-http
Length of output: 333
🏁 Script executed:
find . -name "*websocket*" -type fRepository: firstof9/python-openevse-http
Length of output: 158
🏁 Script executed:
rg "password.*None|pwd.*None" tests/ --type pyRepository: firstof9/python-openevse-http
Length of output: 183
🏁 Script executed:
cat -n openevsehttp/websocket.py | head -60Repository: firstof9/python-openevse-http
Length of output: 2063
🏁 Script executed:
rg "self.*password|self.*pwd" openevsehttp/websocket.py -A 1 -B 1Repository: firstof9/python-openevse-http
Length of output: 263
Normalize pwd=None in the HTTP transport to match websocket behavior.
The websocket transport normalizes None to an empty string (self._password or ""), but Requester passes self._pwd directly to aiohttp.BasicAuth without normalization. Since aiohttp.BasicAuth raises ValueError if password is None, this causes a runtime error for any caller passing pwd=None. Websocket tests rely on this normalization pattern, so HTTP requests should use the same approach.
Suggested fix
- self._pwd = pwd
+ self._pwd = "" if pwd is None else pwdAlso applies to: 56-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openevsehttp/requester.py` around lines 29 - 35, The Requester currently
allows pwd=None which later gets passed directly to aiohttp.BasicAuth and raises
ValueError; update Requester.__init__ (and the code path that constructs
aiohttp.BasicAuth, e.g., where self._pwd is used around the BasicAuth creation)
to normalize None to an empty string (self._pwd = pwd or "") so that callers
passing None behave the same as the websocket transport and no None is forwarded
to aiohttp.BasicAuth.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor / Improvements
Tests
Chores