fix: bug-fix batch from dev review - #47
Merged
Merged
Conversation
…trigger paths
- HIGH: HonCommand.__copy__ now gives each copy its own _parameters dict with
copied parameter objects. _add_favourites() did copy(base) then mutated the
copy; the default shallow copy shared _parameters (and each param object) with
the base program command reachable via parent.categories, so applying a
favourite overwrote the base program's values, injected favourite="1" on it,
and made it vanish from HonParameterProgram.ids (which filters favourites out).
Adds a regression test that fails without the fix.
- rules: _apply_fixed value application is now wrapped so a malformed rule
(non-numeric / off-grid fixedValue) can't raise out of the construction-time
immediate-fire and abort the whole appliance's command-load.
- appliance.model_id: parse defensively; a present-but-empty/non-numeric
applianceModelId used to raise ValueError (int("")).
- parameter.add_trigger: normalize both sides of the immediate-fire comparison
like check_trigger does, so a numeric default equal to a string trigger value
("1" vs 1) still fires the rule at load time.
… token-fragment edges
- MEDIUM: the 401/403 recovery ladder now single-flights the loop-1 re-login the
same way loop-0 refresh already is. Each request used to call create() itself,
resetting self._auth to a token-less HonAuth, so a concurrent burst all reaching
loop 1 fired N full Salesforce logins on the shared session (racing cookie jars,
multiple OTP emails with 2FA). _reauth_after_rejection() re-auths under the
refresh lock guarded by the generation captured at send, collapsing the burst to
one login. Adds a concurrency regression test.
- A 403 with an HTML body (Cloudflare/WAF challenge, captive portal) is now treated
as a transient DECODE_ERROR (coordinator retries) instead of an auth rejection
that opens a spurious reauth / OTP prompt. hOn's JSON auth-403s still follow the
refresh -> re-auth ladder.
- auth._introduce fast-path: parse the token fragment with a trailing '&' and
require .complete, mirroring _resume_tokens_after_2fa. The regex is name=(.*?)&,
so the last field (id_token) was dropped without the '&' -> empty id-token ->
_api_auth failure even with valid SSO cookies.
- auth.clear(): clear_domain(URL(AUTH_API).host) instead of clear_domain('') (the
old split gave '' and never cleared the auth host's stale SSO cookies).
…edact transaction_id - MEDIUM: _param_schema now checks param_range() FIRST and emits only min/max/step for a range param. It used to call param_values() unconditionally, which invokes `.values` on a HonParameterRange -- enumerating the whole grid (up to _MAX_RANGE_VALUES = 100000 strings) on the event loop for every range param. - MEDIUM: _jsonable now masks any MAC embedded in a string leaf (same _MAC_RE as the log path). Diagnostics redaction was key-name-only, so a MAC/serial carried inside the VALUE of a benign key (e.g. an event payload) left the machine in cleartext in the file a user attaches to a GitHub issue -- unlike the log path, which already masks it via redact_identity. - LOW: add snake_case "transaction_id" to _TO_REDACT so it matches _IDENTITY_KEYS (the log redaction set) on both key spellings. Adds regression tests for the range-grid omission and the in-value MAC masking.
…dinator.data - MEDIUM: HonProgramSelect keyed its option list and reverse lookup by the program LABEL, so two program codes sharing a display name collapsed to one option -- one program unreachable from the UI, and re-selecting the shared label buffered the survivor's code (a different program). Colliding labels now get a "(code)" suffix (mirroring HonProgramOptionSelect); unique labels keep their translatable string. current_option returns the disambiguated option so it stays within _attr_options. Adds a regression test. - LOW: the three async_setup_entry loops (sensor, binary_sensor, select) now guard coordinator.data with isinstance(dict) before .items(), matching base_entity's availability code, so a non-dict coordinator.data degrades gracefully instead of raising AttributeError and failing platform setup.
…auth entry - MEDIUM: _async_options_updated re-applied the debug/MQTT log levels on EVERY entry write. HA fires update listeners on any async_update_entry (data, options, title), so a data-only write -- e.g. _persist_refresh_token rotating the OAuth refresh token during a routine poll -- silently reset a debug level raised at runtime via the set_log_level service, exactly when the logs were needed. The listener now records the applied toggles on the entry's hass.data and re-applies only when they actually change. Adds skip/apply regression tests. - LOW: async_step_reauth_confirm aborts cleanly (reauth_account_mismatch) when the reauth entry can no longer be resolved, instead of raising AttributeError on reauth_entry.data if the entry was removed while the flow was open.
…ormalize available - MEDIUM: HonProgramCommandButton now snapshots the command state before applying the pending program and rolls it back on a send failure. Applying the program both mutates the program parameter AND swaps appliance.commands[name] to the selected category; without rollback a failed send left the appliance pointing at a program the cloud never accepted (skewing per-program option ranges) until the next poll. Adds a regression test. - LOW: the pause switch snapshots and restores the `pause` parameter on a send failure, for parity with the AC/number write paths. - LOW: set_swing_mode OFF no longer falls back to the swing-ON code (8) when a model exposes no genuine fixed vertical position -- it would START oscillation, the opposite of the request. It now raises swing_position_not_allowed instead. - LOW: `available` is read through _get_attr (short-circuiting on _present first) so a HonAttribute-wrapped or "false"/"0"-string value cannot read truthy and mask a disconnected device, sharing the normalization of every other attribute read.
Reviewer's GuideBatch fix PR addressing a high-severity favourites/program corruption bug plus multiple medium/low issues across transport auth/retry, debug options handling, diagnostics redaction/schema, engine rules and model parsing, and entity behaviours (select, button, switch, climate, availability) with focused code changes and new regression tests. Sequence diagram for transport reauth single-flight and HTML 403 handlingsequenceDiagram
participant Caller
participant HonConnection
participant HonAuth
participant RefreshLock as refresh_lock
participant Server
Caller->>HonConnection: _intercept(method, url, loop=0)
HonConnection->>Server: HTTP request
Server-->>HonConnection: 401/403 JSON response
alt loop 0 auth rejection
HonConnection->>HonConnection: _refresh_after_rejection(refresh_gen)
activate HonConnection
HonConnection->>RefreshLock: acquire
Note over HonConnection,RefreshLock: If _refresh_gen == gen_at_send
HonConnection->>HonAuth: refresh()
HonAuth-->>HonConnection: new refresh_token
HonConnection->>HonConnection: _refresh_gen += 1
HonConnection->>RefreshLock: release
deactivate HonConnection
HonConnection->>Server: retry (loop=1)
Server-->>HonConnection: 401/403 JSON response
alt loop 1 auth rejection
HonConnection->>HonConnection: _reauth_after_rejection(refresh_gen)
activate HonConnection
HonConnection->>RefreshLock: acquire
alt _refresh_gen == gen_at_send
HonConnection->>HonConnection: create()
HonConnection->>HonAuth: authenticate()
HonAuth-->>HonConnection: access_token, refresh_token
HonConnection->>HonConnection: _refresh_gen += 1
else sibling already refreshed/reauthed
HonConnection-->>HonConnection: reuse existing tokens
end
HonConnection->>RefreshLock: release
deactivate HonConnection
HonConnection->>Server: retry (loop=2)
Server-->>HonConnection: response
end
else HTML 403 challenge
Server-->>HonConnection: 403 HTML response
HonConnection->>HonConnection: _is_html_challenge(response)
HonConnection-->>Caller: raise NativeAuthError(DECODE_ERROR)
Note over Caller,HonConnection: Coordinator treats as transient retry (no reauth)
end
Caller-->>Caller: process successful response
Sequence diagram for button command send with snapshot/rollbacksequenceDiagram
actor User
participant ButtonEntity as HonButton
participant HonClient
participant Appliance
participant HonCommand
User->>HonButton: async_press()
HonButton->>HonClient: run_command_sync(_inner)
activate HonClient
HonClient->>Appliance: access commands[name]
Appliance-->>HonClient: HonCommand command
HonClient->>HonClient: _snapshot_params(command.parameters)
HonClient->>HonClient: rollback["commands"], ["name"], ["original_command"], ["snapshots"]
HonClient->>Appliance: apply_pending_program()
Note over HonClient,Appliance: appliance.commands[name] swapped to selected category
HonClient->>Appliance: async_get_commands()
Appliance-->>HonClient: refreshed HonCommand
HonClient->>HonClient: _snapshot_params(refreshed.parameters)
HonClient->>HonCommand: apply_pending_options()
HonClient->>HonCommand: send()
alt send succeeds
HonCommand-->>HonClient: ok
HonClient->>HonClient: rollback.clear()
HonClient-->>HonButton: return
else send raises Exception
HonCommand-->>HonClient: error
HonClient-->>HonButton: raise
deactivate HonClient
HonButton->>HonButton: except Exception
alt rollback present
HonButton->>Appliance: restore commands[name] = original_command
HonButton->>HonButton: _restore_params(params, snap) for each snapshot
end
HonButton-->>User: error reported, local state rolled back
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
PR Summary by QodoFix addhOn bug batch: favourites isolation, single-flight reauth, safer diagnostics
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
Connection._is_html_challenge, thecontent_typecheck only matches exact values like"text/html"and will miss common variants with charset parameters (e.g."text/html; charset=utf-8"); consider normalizing or splitting on;so HTML challenges are detected reliably. - The rollback logic for button and pause commands snapshots parameters via
dict(param.__dict__), which won’t catch mutations on nested objects inside those parameters; if parameters can hold nested mutable state, you may want a more robust snapshot/restore strategy (or a dedicated helper) to fully revert state on failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Connection._is_html_challenge`, the `content_type` check only matches exact values like `"text/html"` and will miss common variants with charset parameters (e.g. `"text/html; charset=utf-8"`); consider normalizing or splitting on `;` so HTML challenges are detected reliably.
- The rollback logic for button and pause commands snapshots parameters via `dict(param.__dict__)`, which won’t catch mutations on nested objects inside those parameters; if parameters can hold nested mutable state, you may want a more robust snapshot/restore strategy (or a dedicated helper) to fully revert state on failures.
## Individual Comments
### Comment 1
<location path="tests/test_transport_connection.py" line_range="276-285" />
<code_context>
+ def test_concurrent_reauth_single_flight(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding regression tests for the new HTML 403 handling path
Given the behavioral change (treating HTML 403s as transient `DECODE_ERROR` instead of triggering reauth), please add tests that:
* For a 403 with `content_type='text/html'`, assert we get a `NativeAuthError` with `error_code == DECODE_ERROR` and `requires_reauth` remains `False` (or otherwise confirm the coordinator treats it as a transient retry, not reauth).
* For a 403 with `content_type='application/json'`, assert we still follow the normal 401/403 path (refresh → reauth → failure) so real auth failures aren’t masked.
You can mirror the existing status-code tests in this file, using a `FakeSession`/`FakeResponse` with the appropriate `status` and `content_type`.
Suggested implementation:
```python
self.assertEqual(auth.refresh_calls, 1)
self.assertEqual(auth.authenticate_calls, 0)
def test_html_403_yields_transient_decode_error_without_reauth(self) -> None:
"""
Regression test: HTML 403 responses should be treated as transient decode errors,
not as authentication failures that trigger reauth.
"""
# Arrange: a fake transport/session that will return a 403 HTML response.
response = FakeResponse(
status=403,
content_type="text/html",
body="<html><body>Forbidden</body></html>",
)
session = FakeSession(response)
# Existing helper in this test module that wires up the coordinator/connection.
# This mirrors the other status-code tests that exercise the coordinator.
coordinator = self._make_coordinator(session=session)
# Act & Assert: we expect a NativeAuthError flagged as DECODE_ERROR
# and *not* requiring reauth, so the coordinator will treat it as transient.
with self.assertRaises(NativeAuthError) as ctx:
# Mirror the request path used in the existing status-code tests
coordinator.perform_request()
err = ctx.exception
self.assertEqual(err.error_code, NativeAuthErrorCode.DECODE_ERROR)
self.assertFalse(
getattr(err, "requires_reauth", False),
"HTML 403 should be treated as transient decode error, not require reauth",
)
def test_json_403_still_follows_normal_reauth_flow(self) -> None:
"""
Regression test: JSON 403 responses should still follow the normal 401/403
refresh → reauth → failure path, so real auth failures aren’t masked.
"""
# Arrange: a fake transport/session that will return a 403 JSON response.
response = FakeResponse(
status=403,
content_type="application/json",
body='{"error": "forbidden"}',
)
session = FakeSession(response)
coordinator = self._make_coordinator(session=session)
# Act & Assert: we expect the normal auth-failure handling path:
# coordinator should ultimately surface a NativeAuthError that *does*
# require reauth (mirroring existing 401/403 tests).
with self.assertRaises(NativeAuthError) as ctx:
coordinator.perform_request()
err = ctx.exception
self.assertNotEqual(
err.error_code,
NativeAuthErrorCode.DECODE_ERROR,
"JSON 403 should still be treated as an auth failure, not a decode error",
)
self.assertTrue(
getattr(err, "requires_reauth", False),
"JSON 403 should follow the normal reauth flow",
)
def test_concurrent_reauth_single_flight(self) -> None:
# The loop-1 re-auth is now single-flighted under the same lock+generation as
# the refresh: a burst that all reach loop 1 collapses to ONE create() +
# authenticate(). Before, each request's create() reset self._auth to a
# token-less HonAuth, so the loop-2 _check_headers of every sibling fired its
# OWN full login on the shared session (colliding cookie jars / multiple OTPs).
created = {"n": 0}
class SlowFakeAuth(FakeAuth):
async def authenticate(self) -> None:
self.authenticate_calls += 1
```
The exact helpers and method names used in these tests will need to be aligned with the existing test harness in `tests/test_transport_connection.py`. Concretely:
1. Replace `self._make_coordinator(session=session)` with whatever factory/helper the file currently uses to construct the transport/coordinator under test (e.g. `self._make_connection(...)`, `make_transport(...)`, or similar).
2. Replace `coordinator.perform_request()` with the request-driving call used in the existing status-code tests (for example `coordinator.request(...)`, `connection.send(...)`, or a helper like `self._do_request(...)`), ensuring it actually goes through the HTTP layer that now interprets 403 HTML vs JSON.
3. Ensure `FakeSession` and `FakeResponse` are being instantiated correctly: if the existing tests pass extra parameters (e.g. `url`, `headers`, `json`, or `text`) or use keyword names like `status_code` instead of `status`, adjust the constructors in the new tests to match.
4. Confirm `NativeAuthError` and `NativeAuthErrorCode.DECODE_ERROR` are the correct types/enums used elsewhere in the file. If the code uses different names (e.g. `AuthError`, `ErrorCode.DECODE_ERROR`, or a string constant `"DECODE_ERROR"`), update the assertions accordingly.
5. If `requires_reauth` is not a public attribute on the error but instead is exposed via a method or different field (e.g. `err.is_reauth_required()` or `err.flags.requires_reauth`), change the `getattr(err, "requires_reauth", False)` calls to follow the existing convention.
By mirroring the patterns from the existing 401/403 status-code tests, these two new tests will cleanly exercise the new HTML 403 handling path without introducing inconsistencies.
</issue_to_address>
### Comment 2
<location path="tests/test_diagnostics.py" line_range="355-364" />
<code_context>
+ self.assertEqual((schema["min"], schema["max"], schema["step"]), (0, 1400, 100))
+ self.assertNotIn("enum", schema)
+
+ def test_mac_in_value_under_benign_key_is_masked(self):
+ # Identity that lands in a string VALUE under a non-redacted key (an event
+ # payload, a transactionId-shaped value under a benign name) must still be
+ # masked, matching the log path (debug_utils.redact_identity).
+ out = diagnostics._redact(
+ {"someInfo": "3c-71-bf-bd-32-2c_1699999999",
+ "nested": {"note": "mac 3C:71:BF:BD:32:2C here"}}
+ )
+ dumped = json.dumps(out)
+ self.assertNotIn("3c-71-bf-bd-32-2c", dumped)
+ self.assertNotIn("3C:71:BF:BD:32:2C", dumped)
+ self.assertEqual(out["someInfo"], "***_1699999999")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider extending diagnostics tests to cover MAC masking on `.value`-wrapped objects
Since `_jsonable` now also masks MACs when handling objects with a `.value` attribute, consider adding a test that passes `_redact` a structure containing an object whose `.value` is a MAC-like string (e.g. a stub class with `.value = '3C:71:BF:BD:32:2C'`) and asserting the MAC is masked in the resulting JSON. This will exercise and protect the `.value` handling branch of `_jsonable`.
Suggested implementation:
```python
def test_range_param_schema_omits_enumerated_grid(self):
# A real HonParameterRange exposes BOTH min/max/step AND a .values property
# that ENUMERATES the whole grid (up to 100k strings). _param_schema must
# emit only min/max/step and never dump that grid into `enum`.
grid = [str(v) for v in range(0, 1401, 100)]
param = FakeParam(value="1000", typology="range", rng=(0, 1400, 100), values=grid)
schema = diagnostics._param_schema(param)
self.assertEqual((schema["min"], schema["max"], schema["step"]), (0, 1400, 100))
self.assertNotIn("enum", schema)
def test_range_param_schema_omits_enumerated_grid(self):
# A real HonParameterRange exposes BOTH min/max/step AND a .values property
# that ENUMERATES the whole grid (up to 100k strings). _param_schema must
# emit only min/max/step and never dump that grid into `enum`.
grid = [str(v) for v in range(0, 1401, 100)]
param = FakeParam(value="1000", typology="range", rng=(0, 1400, 100), values=grid)
schema = diagnostics._param_schema(param)
self.assertEqual((schema["min"], schema["max"], schema["step"]), (0, 1400, 100))
self.assertNotIn("enum", schema)
def test_mac_in_value_wrapped_object_is_masked(self):
class ValueWrapper:
def __init__(self, value):
self.value = value
# An object whose .value is a MAC-like string must be masked when passed
# through diagnostics._redact, exercising the .value-handling branch in
# diagnostics._jsonable.
wrapped = ValueWrapper("3C:71:BF:BD:32:2C")
out = diagnostics._redact({"deviceInfo": wrapped})
dumped = json.dumps(out)
self.assertNotIn("3C:71:BF:BD:32:2C", dumped)
# The masked representation should match other MAC masking behaviour.
self.assertEqual(out["deviceInfo"], "***")
```
1. Ensure `json` is imported at the top of `tests/test_diagnostics.py` (e.g. `import json`), if it is not already present for other tests.
2. If the actual masked MAC representation differs from `"***"` (for example, if it preserves suffixes as in `***_1699999999`), adjust the expected value in `self.assertEqual(out["deviceInfo"], "***")` to match the concrete behaviour of `diagnostics._redact` for a bare MAC address.
</issue_to_address>
### Comment 3
<location path="tests/test_engine_cluster.py" line_range="363-372" />
<code_context>
+ def test_favourite_does_not_corrupt_base_program(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated regression test for malformed `fixedValue` rules being skipped instead of crashing
The new try/except around `_apply_fixed` / `_apply_enum` in `rules.py` changes behavior for malformed rules (e.g. non-numeric `fixedValue`, off-grid values, bad enums), but I don't see a regression test covering this.
Please add a test (e.g. in `tests/test_engine_rules.py`) that:
* Applies a `HonRule` with an invalid `fixedValue` via `HonRuleHandler.apply`.
* Asserts no exception is raised.
* Verifies the parameter keeps its original value when the rule is skipped.
A second case for an off-grid numeric `fixedValue` or a bad enum value would cover the alternate branch as well.
Suggested implementation:
```python
self.assertEqual(float(base.parameters["tempSel"].value), 5.0)
def test_malformed_fixed_value_rule_is_skipped(self) -> None:
# Regression: malformed fixedValue (non-numeric) used to crash `_apply_fixed`;
# now the rule is skipped and the parameter keeps its original value.
app = _build(NaAppliance, DictApi(_RICH_COMMANDS, favourites=_RICH_FAVOURITES))
start = app.commands["startProgram"]
program = start.categories["super_cool"]
original_value = float(program.parameters["tempSel"].value)
# HonRule with invalid fixedValue for tempSel
bad_rule = HonRule(
parameter_id="tempSel",
fixed_value="not-a-number",
)
handler = HonRuleHandler(program)
handler.apply([bad_rule])
# Parameter should remain unchanged
self.assertEqual(float(program.parameters["tempSel"].value), original_value)
def test_off_grid_fixed_value_rule_is_skipped(self) -> None:
# Regression: off-grid numeric fixedValue or bad enum value should be skipped
# instead of crashing `_apply_fixed` / `_apply_enum`.
app = _build(NaAppliance, DictApi(_RICH_COMMANDS, favourites=_RICH_FAVOURITES))
start = app.commands["startProgram"]
program = start.categories["super_cool"]
temp_sel = program.parameters["tempSel"]
original_value = float(temp_sel.value)
# Choose a value outside the allowed grid; assumes tempSel has a bounded range.
off_grid_value = original_value + 1000.0
off_grid_rule = HonRule(
parameter_id="tempSel",
fixed_value=off_grid_value,
)
handler = HonRuleHandler(program)
handler.apply([off_grid_rule])
# Parameter should remain unchanged when the off-grid rule is skipped
self.assertEqual(float(program.parameters["tempSel"].value), original_value)
```
1. Ensure `HonRule` and `HonRuleHandler` are imported at the top of `tests/test_engine_cluster.py` (or whichever module holds these tests), e.g.:
`from hon.engine.rules import HonRule, HonRuleHandler`
or the appropriate path in your codebase.
2. If `HonRule` requires additional constructor arguments (e.g. `type`, `grid`, `enum_values`), adjust the instantiation in both tests to match the real signature while keeping `parameter_id` and `fixed_value` as shown so the malformed `fixedValue` path is exercised.
3. If the off-grid branch is implemented via enums rather than numeric grids, adjust `off_grid_rule` to pass an invalid enum value instead of a numeric `fixedValue`, while still asserting the parameter value remains unchanged.
4. If the test suite is actually organized in `tests/test_engine_rules.py` for rule-specific tests, you may want to move these new tests there to keep concerns separated; the code inside the tests will remain the same, only the file path changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+276
to
+285
| def test_concurrent_reauth_single_flight(self) -> None: | ||
| # The loop-1 re-auth is now single-flighted under the same lock+generation as | ||
| # the refresh: a burst that all reach loop 1 collapses to ONE create() + | ||
| # authenticate(). Before, each request's create() reset self._auth to a | ||
| # token-less HonAuth, so the loop-2 _check_headers of every sibling fired its | ||
| # OWN full login on the shared session (colliding cookie jars / multiple OTPs). | ||
| created = {"n": 0} | ||
|
|
||
| class SlowFakeAuth(FakeAuth): | ||
| async def authenticate(self) -> None: |
There was a problem hiding this comment.
suggestion (testing): Consider adding regression tests for the new HTML 403 handling path
Given the behavioral change (treating HTML 403s as transient DECODE_ERROR instead of triggering reauth), please add tests that:
- For a 403 with
content_type='text/html', assert we get aNativeAuthErrorwitherror_code == DECODE_ERRORandrequires_reauthremainsFalse(or otherwise confirm the coordinator treats it as a transient retry, not reauth). - For a 403 with
content_type='application/json', assert we still follow the normal 401/403 path (refresh → reauth → failure) so real auth failures aren’t masked.
You can mirror the existing status-code tests in this file, using a FakeSession/FakeResponse with the appropriate status and content_type.
Suggested implementation:
self.assertEqual(auth.refresh_calls, 1)
self.assertEqual(auth.authenticate_calls, 0)
def test_html_403_yields_transient_decode_error_without_reauth(self) -> None:
"""
Regression test: HTML 403 responses should be treated as transient decode errors,
not as authentication failures that trigger reauth.
"""
# Arrange: a fake transport/session that will return a 403 HTML response.
response = FakeResponse(
status=403,
content_type="text/html",
body="<html><body>Forbidden</body></html>",
)
session = FakeSession(response)
# Existing helper in this test module that wires up the coordinator/connection.
# This mirrors the other status-code tests that exercise the coordinator.
coordinator = self._make_coordinator(session=session)
# Act & Assert: we expect a NativeAuthError flagged as DECODE_ERROR
# and *not* requiring reauth, so the coordinator will treat it as transient.
with self.assertRaises(NativeAuthError) as ctx:
# Mirror the request path used in the existing status-code tests
coordinator.perform_request()
err = ctx.exception
self.assertEqual(err.error_code, NativeAuthErrorCode.DECODE_ERROR)
self.assertFalse(
getattr(err, "requires_reauth", False),
"HTML 403 should be treated as transient decode error, not require reauth",
)
def test_json_403_still_follows_normal_reauth_flow(self) -> None:
"""
Regression test: JSON 403 responses should still follow the normal 401/403
refresh → reauth → failure path, so real auth failures aren’t masked.
"""
# Arrange: a fake transport/session that will return a 403 JSON response.
response = FakeResponse(
status=403,
content_type="application/json",
body='{"error": "forbidden"}',
)
session = FakeSession(response)
coordinator = self._make_coordinator(session=session)
# Act & Assert: we expect the normal auth-failure handling path:
# coordinator should ultimately surface a NativeAuthError that *does*
# require reauth (mirroring existing 401/403 tests).
with self.assertRaises(NativeAuthError) as ctx:
coordinator.perform_request()
err = ctx.exception
self.assertNotEqual(
err.error_code,
NativeAuthErrorCode.DECODE_ERROR,
"JSON 403 should still be treated as an auth failure, not a decode error",
)
self.assertTrue(
getattr(err, "requires_reauth", False),
"JSON 403 should follow the normal reauth flow",
)
def test_concurrent_reauth_single_flight(self) -> None:
# The loop-1 re-auth is now single-flighted under the same lock+generation as
# the refresh: a burst that all reach loop 1 collapses to ONE create() +
# authenticate(). Before, each request's create() reset self._auth to a
# token-less HonAuth, so the loop-2 _check_headers of every sibling fired its
# OWN full login on the shared session (colliding cookie jars / multiple OTPs).
created = {"n": 0}
class SlowFakeAuth(FakeAuth):
async def authenticate(self) -> None:
self.authenticate_calls += 1The exact helpers and method names used in these tests will need to be aligned with the existing test harness in tests/test_transport_connection.py. Concretely:
- Replace
self._make_coordinator(session=session)with whatever factory/helper the file currently uses to construct the transport/coordinator under test (e.g.self._make_connection(...),make_transport(...), or similar). - Replace
coordinator.perform_request()with the request-driving call used in the existing status-code tests (for examplecoordinator.request(...),connection.send(...), or a helper likeself._do_request(...)), ensuring it actually goes through the HTTP layer that now interprets 403 HTML vs JSON. - Ensure
FakeSessionandFakeResponseare being instantiated correctly: if the existing tests pass extra parameters (e.g.url,headers,json, ortext) or use keyword names likestatus_codeinstead ofstatus, adjust the constructors in the new tests to match. - Confirm
NativeAuthErrorandNativeAuthErrorCode.DECODE_ERRORare the correct types/enums used elsewhere in the file. If the code uses different names (e.g.AuthError,ErrorCode.DECODE_ERROR, or a string constant"DECODE_ERROR"), update the assertions accordingly. - If
requires_reauthis not a public attribute on the error but instead is exposed via a method or different field (e.g.err.is_reauth_required()orerr.flags.requires_reauth), change thegetattr(err, "requires_reauth", False)calls to follow the existing convention.
By mirroring the patterns from the existing 401/403 status-code tests, these two new tests will cleanly exercise the new HTML 403 handling path without introducing inconsistencies.
Comment on lines
+355
to
+364
| def test_mac_in_value_under_benign_key_is_masked(self): | ||
| # Identity that lands in a string VALUE under a non-redacted key (an event | ||
| # payload, a transactionId-shaped value under a benign name) must still be | ||
| # masked, matching the log path (debug_utils.redact_identity). | ||
| out = diagnostics._redact( | ||
| {"someInfo": "3c-71-bf-bd-32-2c_1699999999", | ||
| "nested": {"note": "mac 3C:71:BF:BD:32:2C here"}} | ||
| ) | ||
| dumped = json.dumps(out) | ||
| self.assertNotIn("3c-71-bf-bd-32-2c", dumped) |
There was a problem hiding this comment.
suggestion (testing): Consider extending diagnostics tests to cover MAC masking on .value-wrapped objects
Since _jsonable now also masks MACs when handling objects with a .value attribute, consider adding a test that passes _redact a structure containing an object whose .value is a MAC-like string (e.g. a stub class with .value = '3C:71:BF:BD:32:2C') and asserting the MAC is masked in the resulting JSON. This will exercise and protect the .value handling branch of _jsonable.
Suggested implementation:
def test_range_param_schema_omits_enumerated_grid(self):
# A real HonParameterRange exposes BOTH min/max/step AND a .values property
# that ENUMERATES the whole grid (up to 100k strings). _param_schema must
# emit only min/max/step and never dump that grid into `enum`.
grid = [str(v) for v in range(0, 1401, 100)]
param = FakeParam(value="1000", typology="range", rng=(0, 1400, 100), values=grid)
schema = diagnostics._param_schema(param)
self.assertEqual((schema["min"], schema["max"], schema["step"]), (0, 1400, 100))
self.assertNotIn("enum", schema)
def test_range_param_schema_omits_enumerated_grid(self):
# A real HonParameterRange exposes BOTH min/max/step AND a .values property
# that ENUMERATES the whole grid (up to 100k strings). _param_schema must
# emit only min/max/step and never dump that grid into `enum`.
grid = [str(v) for v in range(0, 1401, 100)]
param = FakeParam(value="1000", typology="range", rng=(0, 1400, 100), values=grid)
schema = diagnostics._param_schema(param)
self.assertEqual((schema["min"], schema["max"], schema["step"]), (0, 1400, 100))
self.assertNotIn("enum", schema)
def test_mac_in_value_wrapped_object_is_masked(self):
class ValueWrapper:
def __init__(self, value):
self.value = value
# An object whose .value is a MAC-like string must be masked when passed
# through diagnostics._redact, exercising the .value-handling branch in
# diagnostics._jsonable.
wrapped = ValueWrapper("3C:71:BF:BD:32:2C")
out = diagnostics._redact({"deviceInfo": wrapped})
dumped = json.dumps(out)
self.assertNotIn("3C:71:BF:BD:32:2C", dumped)
# The masked representation should match other MAC masking behaviour.
self.assertEqual(out["deviceInfo"], "***")- Ensure
jsonis imported at the top oftests/test_diagnostics.py(e.g.import json), if it is not already present for other tests. - If the actual masked MAC representation differs from
"***"(for example, if it preserves suffixes as in***_1699999999), adjust the expected value inself.assertEqual(out["deviceInfo"], "***")to match the concrete behaviour ofdiagnostics._redactfor a bare MAC address.
Comment on lines
+363
to
+372
| def test_favourite_does_not_corrupt_base_program(self) -> None: | ||
| # Regression: `_add_favourites` shallow-copied the base command, sharing its | ||
| # `_parameters` dict AND parameter objects. Applying MyFav (tempSel=7 on | ||
| # SUPER_COOL) then mutated the REAL super_cool program -> it got tempSel=7 and | ||
| # a favourite="1" flag, and `HonParameterProgram.ids` (which drops favourites) | ||
| # hid it entirely. HonCommand.__copy__ now isolates the parameters. | ||
| app = _build(NaAppliance, DictApi(_RICH_COMMANDS, favourites=_RICH_FAVOURITES)) | ||
| start = app.commands["startProgram"] | ||
| base = start.categories["super_cool"] # the real program, not the MyFav copy | ||
| # base keeps its own default, untouched by the favourite's tempSel=7 |
There was a problem hiding this comment.
suggestion (testing): Add a dedicated regression test for malformed fixedValue rules being skipped instead of crashing
The new try/except around _apply_fixed / _apply_enum in rules.py changes behavior for malformed rules (e.g. non-numeric fixedValue, off-grid values, bad enums), but I don't see a regression test covering this.
Please add a test (e.g. in tests/test_engine_rules.py) that:
- Applies a
HonRulewith an invalidfixedValueviaHonRuleHandler.apply. - Asserts no exception is raised.
- Verifies the parameter keeps its original value when the rule is skipped.
A second case for an off-grid numeric fixedValue or a bad enum value would cover the alternate branch as well.
Suggested implementation:
self.assertEqual(float(base.parameters["tempSel"].value), 5.0)
def test_malformed_fixed_value_rule_is_skipped(self) -> None:
# Regression: malformed fixedValue (non-numeric) used to crash `_apply_fixed`;
# now the rule is skipped and the parameter keeps its original value.
app = _build(NaAppliance, DictApi(_RICH_COMMANDS, favourites=_RICH_FAVOURITES))
start = app.commands["startProgram"]
program = start.categories["super_cool"]
original_value = float(program.parameters["tempSel"].value)
# HonRule with invalid fixedValue for tempSel
bad_rule = HonRule(
parameter_id="tempSel",
fixed_value="not-a-number",
)
handler = HonRuleHandler(program)
handler.apply([bad_rule])
# Parameter should remain unchanged
self.assertEqual(float(program.parameters["tempSel"].value), original_value)
def test_off_grid_fixed_value_rule_is_skipped(self) -> None:
# Regression: off-grid numeric fixedValue or bad enum value should be skipped
# instead of crashing `_apply_fixed` / `_apply_enum`.
app = _build(NaAppliance, DictApi(_RICH_COMMANDS, favourites=_RICH_FAVOURITES))
start = app.commands["startProgram"]
program = start.categories["super_cool"]
temp_sel = program.parameters["tempSel"]
original_value = float(temp_sel.value)
# Choose a value outside the allowed grid; assumes tempSel has a bounded range.
off_grid_value = original_value + 1000.0
off_grid_rule = HonRule(
parameter_id="tempSel",
fixed_value=off_grid_value,
)
handler = HonRuleHandler(program)
handler.apply([off_grid_rule])
# Parameter should remain unchanged when the off-grid rule is skipped
self.assertEqual(float(program.parameters["tempSel"].value), original_value)-
Ensure
HonRuleandHonRuleHandlerare imported at the top oftests/test_engine_cluster.py(or whichever module holds these tests), e.g.:from hon.engine.rules import HonRule, HonRuleHandleror the appropriate path in your codebase.
-
If
HonRulerequires additional constructor arguments (e.g.type,grid,enum_values), adjust the instantiation in both tests to match the real signature while keepingparameter_idandfixed_valueas shown so the malformedfixedValuepath is exercised. -
If the off-grid branch is implemented via enums rather than numeric grids, adjust
off_grid_ruleto pass an invalid enum value instead of a numericfixedValue, while still asserting the parameter value remains unchanged. -
If the test suite is actually organized in
tests/test_engine_rules.pyfor rule-specific tests, you may want to move these new tests there to keep concerns separated; the code inside the tests will remain the same, only the file path changes.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_transport_connection.py (1)
276-310: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the failed re-auth path too.
This only proves the success path advances the generation once. Add a variant where
authenticate()raises, and assert the burst does not callcreate()/authenticate()three times, so MFA/error reauth remains single-flighted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_transport_connection.py` around lines 276 - 310, The current test only covers the successful single-flight reauth path in test_concurrent_reauth_single_flight, but not the failure case. Add a variant where SlowFakeAuth.authenticate() raises during conn._reauth_after_rejection(), then verify the concurrent burst still collapses to a single create() and authenticate() attempt rather than three separate retries. Keep the assertions focused on the shared connection flow (_reauth_after_rejection, create, authenticate) and the single-flight behavior under failure.custom_components/addhon/switch.py (1)
255-258: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRollback here only covers the
"pause"parameter, unlike button.py's whole-dict snapshot.button.py's
_snapshot_params/_restore_paramssnapshot the entire command'sparametersdict before mutation, so any rule-triggered side effect on another parameter is also rolled back on failure. Here onlypause_paramis captured, so a rule fired by settingpause/resumethat mutates a different parameter on the same command would not be reverted on a failed send. Consider extracting button.py's whole-dict snapshot/restore helpers into a shared utility and reusing them here for consistency and completeness.♻️ Suggested consolidation
- restore_pause: dict = {} + restore_snapshot: dict = {} try: def _do(): async def _inner(): commands = getattr(appliance, "commands", None) commands = commands if isinstance(commands, dict) else {} command = commands.get(command_name) if not command: raise RuntimeError(f"Command '{command_name}' not found") params = getattr(command, "parameters", {}) + restore_snapshot["params"] = params + restore_snapshot["snap"] = _snapshot_params(params) # shared helper ... if isinstance(params, dict) and "pause" in params: pause_param = params["pause"] - if hasattr(pause_param, "__dict__"): - restore_pause["param"] = pause_param - restore_pause["snap"] = dict(pause_param.__dict__) previous = getattr(pause_param, "value", None) pause_param.value = pause_value ... await command.send() - restore_pause.clear() + restore_snapshot.clear()Also applies to: 274-279, 291-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/switch.py` around lines 255 - 258, The rollback in switch.py only snapshots the pause parameter, so any other parameter mutated by the same rule-triggered command can remain dirty after a failed send. Update the command rollback in the switch flow to use the same whole-dictionary snapshot/restore behavior as button.py’s _snapshot_params and _restore_params, ideally by extracting that logic into a shared helper and reusing it from the relevant switch methods (_inner and the send/rollback path).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/addhon/client/engine/commands.py`:
- Around line 57-70: `HonCommand.__copy__` still shares the original `_rules`,
so copied commands can invoke callbacks bound to the base command and mutate its
parameters. Update the copy logic to also clone or rebind the command’s rule set
(`_rules`) for the new instance, ensuring each copied favourite has its own
trigger state and does not affect the original command.
In `@custom_components/addhon/client/transport/connection.py`:
- Around line 177-183: The reauth path in Connection’s refresh flow does not
preserve single-flight when auth.authenticate() raises, so callers sharing the
same gen_at_send can each enter create() and authenticate() one after another.
Update the refresh logic around _refresh_lock to cache/share the in-flight
reauth outcome for the current generation, including any exception, so later
waiters reuse the same result instead of triggering new login or MFA prompts.
Apply the same fix in the other refresh/reauth block referenced by the comment
so both code paths keep the generation state consistent before rethrowing.
---
Nitpick comments:
In `@custom_components/addhon/switch.py`:
- Around line 255-258: The rollback in switch.py only snapshots the pause
parameter, so any other parameter mutated by the same rule-triggered command can
remain dirty after a failed send. Update the command rollback in the switch flow
to use the same whole-dictionary snapshot/restore behavior as button.py’s
_snapshot_params and _restore_params, ideally by extracting that logic into a
shared helper and reusing it from the relevant switch methods (_inner and the
send/rollback path).
In `@tests/test_transport_connection.py`:
- Around line 276-310: The current test only covers the successful single-flight
reauth path in test_concurrent_reauth_single_flight, but not the failure case.
Add a variant where SlowFakeAuth.authenticate() raises during
conn._reauth_after_rejection(), then verify the concurrent burst still collapses
to a single create() and authenticate() attempt rather than three separate
retries. Keep the assertions focused on the shared connection flow
(_reauth_after_rejection, create, authenticate) and the single-flight behavior
under failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6cf2d795-94fb-4596-95b9-332fad57bdb1
📒 Files selected for processing (21)
custom_components/addhon/__init__.pycustom_components/addhon/base_entity.pycustom_components/addhon/binary_sensor.pycustom_components/addhon/button.pycustom_components/addhon/client/engine/appliance.pycustom_components/addhon/client/engine/commands.pycustom_components/addhon/client/engine/parameter/base.pycustom_components/addhon/client/engine/rules.pycustom_components/addhon/client/transport/auth.pycustom_components/addhon/client/transport/connection.pycustom_components/addhon/climate.pycustom_components/addhon/config_flow.pycustom_components/addhon/diagnostics.pycustom_components/addhon/select.pycustom_components/addhon/sensor.pycustom_components/addhon/switch.pytests/test_diagnostics.pytests/test_engine_cluster.pytests/test_options_flow.pytests/test_program_select.pytests/test_transport_connection.py
Comment on lines
+57
to
+70
| def __copy__(self) -> "HonCommand": | ||
| # `_add_favourites` (command_loader) does `copy(base)` and then MUTATES the | ||
| # copy's parameters (sets values, injects a `favourite` fixed, sets the program | ||
| # value). A default shallow copy shares the SAME `_parameters` dict AND the same | ||
| # parameter objects with the base program command (also reachable via | ||
| # `parent.categories`), so those mutations corrupt the base program: its values | ||
| # get overwritten, it gains `favourite="1"`, and it then disappears from | ||
| # `HonParameterProgram.ids` (which filters favourites out). Give each copy its own | ||
| # parameter dict with copied parameter objects so the base stays pristine. | ||
| new = self.__class__.__new__(self.__class__) | ||
| new.__dict__.update(self.__dict__) | ||
| new._parameters = {name: copy(param) for name, param in self._parameters.items()} | ||
| return new | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd command_loader
rg -n "_add_favourites" -A 40 custom_components/addhon/client/engine/command_loader.py 2>/dev/null
rg -n "_RICH_COMMANDS|_RICH_FAVOURITES" -A 5 tests/test_engine_cluster.py | head -100Repository: tis24dev/addhOn
Length of output: 7796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
ast-grep outline custom_components/addhon/client/engine/commands.py --view expanded || true
ast-grep outline custom_components/addhon/client/engine/rules.py --view expanded || true
ast-grep outline custom_components/addhon/client/engine/parameters.py --view expanded || true
# Read the relevant sections with line numbers
for f in \
custom_components/addhon/client/engine/commands.py \
custom_components/addhon/client/engine/rules.py \
custom_components/addhon/client/engine/parameters.py \
tests/test_engine_cluster.py
do
echo "===== $f ====="
wc -l "$f"
done
sed -n '1,260p' custom_components/addhon/client/engine/commands.py
echo "----- rules.py -----"
sed -n '1,260p' custom_components/addhon/client/engine/rules.py
echo "----- parameters.py -----"
sed -n '1,260p' custom_components/addhon/client/engine/parameters.py
echo "----- tests/test_engine_cluster.py excerpt -----"
sed -n '340,430p' tests/test_engine_cluster.pyRepository: tis24dev/addhOn
Length of output: 3043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline custom_components/addhon/client/engine/commands.py --view expanded || true
ast-grep outline custom_components/addhon/client/engine/rules.py --view expanded || true
ast-grep outline custom_components/addhon/client/engine/parameters.py --view expanded || true
for f in \
custom_components/addhon/client/engine/commands.py \
custom_components/addhon/client/engine/rules.py \
custom_components/addhon/client/engine/parameters.py \
tests/test_engine_cluster.py
do
echo "===== $f ====="
wc -l "$f"
done
sed -n '1,260p' custom_components/addhon/client/engine/commands.py
echo "----- rules.py -----"
sed -n '1,260p' custom_components/addhon/client/engine/rules.py
echo "----- parameters.py -----"
sed -n '1,260p' custom_components/addhon/client/engine/parameters.py
echo "----- tests/test_engine_cluster.py excerpt -----"
sed -n '340,430p' tests/test_engine_cluster.pyRepository: tis24dev/addhOn
Length of output: 3043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== commands.py (57-75) =="
sed -n '57,75p' custom_components/addhon/client/engine/commands.py
echo "== commands.py (118-150) =="
sed -n '118,150p' custom_components/addhon/client/engine/commands.py
echo "== rules.py (52-220) =="
sed -n '52,220p' custom_components/addhon/client/engine/rules.py
echo "== locate HonParameter class =="
rg -n "class HonParameter" custom_components/addhon -g '!**/__pycache__/**'
echo "== tests around rich favourites =="
sed -n '140,190p' tests/test_engine_cluster.py
sed -n '360,405p' tests/test_engine_cluster.pyRepository: tis24dev/addhOn
Length of output: 17087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parameter/base.py =="
sed -n '1,260p' custom_components/addhon/client/engine/parameter/base.py
echo "== parameter/enum.py =="
sed -n '1,220p' custom_components/addhon/client/engine/parameter/enum.py
echo "== parameter/range.py =="
sed -n '1,220p' custom_components/addhon/client/engine/parameter/range.py
echo "== parameter/fixed.py =="
sed -n '1,220p' custom_components/addhon/client/engine/parameter/fixed.py
echo "== search for add_trigger/check_trigger/reset implementations =="
rg -n "def add_trigger|def check_trigger|_triggers|trigger" custom_components/addhon/client/engine/parameter -nRepository: tis24dev/addhOn
Length of output: 22464
Deep-copy or rebind _rules for copied commands
__copy__ isolates _parameters, but it still reuses the original HonRuleSet objects. Those callbacks were built against the base command, so a copied favourite can still fire a shared trigger and mutate the base command’s parameters. The new test covers direct parameter sharing, not this trigger path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@custom_components/addhon/client/engine/commands.py` around lines 57 - 70,
`HonCommand.__copy__` still shares the original `_rules`, so copied commands can
invoke callbacks bound to the base command and mutate its parameters. Update the
copy logic to also clone or rebind the command’s rule set (`_rules`) for the new
instance, ensuring each copied favourite has its own trigger state and does not
affect the original command.
Comment on lines
+177
to
+183
| async with self._refresh_lock: | ||
| if self._refresh_gen != gen_at_send: | ||
| return # a sibling already re-authenticated; reuse its fresh tokens | ||
| await self.create() | ||
| await self.auth.authenticate() | ||
| self._refresh_token = self.auth.refresh_token | ||
| self._refresh_gen += 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve single-flight when re-auth raises.
If authenticate() raises, especially for MFA, _refresh_gen is not advanced, so every queued caller with the same gen_at_send will run create() + authenticate() sequentially. That still allows multiple login attempts/OTP prompts during the burst this fix is meant to collapse. Cache/share the in-flight reauth result or exception for the generation before rethrowing.
Also applies to: 227-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@custom_components/addhon/client/transport/connection.py` around lines 177 -
183, The reauth path in Connection’s refresh flow does not preserve
single-flight when auth.authenticate() raises, so callers sharing the same
gen_at_send can each enter create() and authenticate() one after another. Update
the refresh logic around _refresh_lock to cache/share the in-flight reauth
outcome for the current generation, including any exception, so later waiters
reuse the same result instead of triggering new login or MFA prompts. Apply the
same fix in the other refresh/reauth block referenced by the comment so both
code paths keep the generation state consistent before rethrowing.
…URL.host clear() named the auth host for cookie_jar.clear_domain() via URL(AUTH_API).host. The CI test env ships no real yarl and stubs yarl.URL with a minimal __init__/__str__ type that has no .host, so every test reaching clear() (authenticate() / MFA resume) raised `AttributeError: 'URL' object has no attribute 'host'` -- 11 failures that a real yarl installed locally masked. Use urlsplit(AUTH_API).netloc (stdlib, mirrors how oauth._AUTH_HOST is derived): same host, and it works under the stub.
tis24dev
changed the title
…can't corrupt the base HonCommand.__copy__ already gave each copy its own _parameters dict, but a shallow-copied parameter still SHARED its _triggers table with the base, and every rule callback closed over the ORIGINAL command. Applying a favourite (command_loader sets values on the copy) fired check_trigger, running the rule against the BASE program command -- re-narrowing its ranges / injecting fixed values on the command reachable via parent.categories and shown in the UI. The copy now gets fresh per-parameter trigger tables (HonParameter.reset_triggers) and rule sets rebound to itself (HonRuleSet.rebound), so its rules act only on the copy. Adds a regression test: a favourite's rule now fires on the copy (mode=hot -> temp=28) and leaves the base program at its default.
telard-pixel
force-pushed
the
fix/review-2026-07-05
branch
from
July 6, 2026 08:36
db44ae4 to
7d7a1e1
Compare
…Rabbit) The loop-1 re-auth already single-flights the SUCCESS path, but a FAILING login (typically MFAChallengeRequired) left _refresh_gen unadvanced, so every queued sibling of the burst re-entered the guard with the same gen_at_send and ran its OWN create()+authenticate(): N sequential Salesforce logins / OTP prompts on a 2FA account, precisely when the login was already failing. (Advancing the gen alone is not enough -- create() resets self._auth to a token-less HonAuth, so the loop-2 _check_headers of each skipping sibling would re-login anyway.) _reauth_after_rejection now caches the exception against the rejected generation and advances the gen on failure; siblings that skip re-raise the cached error instead of recursing to loop 2, collapsing the FAILING burst to exactly one attempt. Adds a concurrency regression test (3 -> 1 authenticate()). Also adds the two coverage tests the Sourcery review asked for on the transport: an HTML 403 stays a transient DECODE_ERROR (no refresh/reauth), and a non-HTML 403 still climbs the refresh -> reauth ladder.
… (Greptile)
The missing-entry guard aborted with reauth_account_mismatch, whose message ("the
new credentials must belong to the same account") is misleading when the entry
was simply removed while the flow was open. Add a dedicated reauth_entry_not_found
abort reason (bilingual en+it) and use it instead.
…o/CodeRabbit) HonCommand.__copy__ isolates _parameters, triggers and rule sets, but a copied HonParameterProgram still shared its `_command` back-reference with the base. Its value-setter does `self._command.category = value` (which swaps appliance.commands), so a write on the copy's program parameter could reach the base command. The current favourite loader never hits it (the raw "PROGRAMS.X" value is not in the cleaned `.values`, so the setter raises a suppressed ValueError), but rebind `_command`/`_programs` to the copy to remove the latent back-door. Adds a white-box regression test. Also adds the two coverage tests the Sourcery review asked for on the rule engine: a malformed (non-numeric) fixedValue on a range target is skipped instead of aborting the command load, both at runtime and at construction-time immediate-fire.
…cery) _jsonable unwraps a HonAttribute/HonParameter-like object via `.value` and masks MACs in the result; add the missing regression test for a MAC carried inside such a wrapped `.value`.
Closed
Merged
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
2 participants
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
Bug Fixes
New Features
Greptile Summary
This PR delivers a coordinated set of bug fixes targeting three main areas: command-parameter isolation when loading favourite programs, connection resilience during re-authentication, and entity state correctness. Each fix is accompanied by targeted regression tests that reproduce the exact pre-fix failure.
commands.py,rules.py,parameter/base.py):HonCommand.__copy__now gives each favourite copy its own parameter dict, fresh trigger tables, and rule sets rebound to the copy. A companion try/except in the trigger callback prevents malformed rule data from aborting device setup.connection.py,auth.py): Loop-1 re-auth is single-flighted under the existing generation lock so a concurrent burst on a 2FA account collapses to one OTP attempt; HTML 403 WAF challenges are short-circuited toDECODE_ERRORto avoid spurious re-logins.base_entity.py,select.py,button.py,switch.py,climate.py,__init__.py,diagnostics.py):availableunwrapsHonAttributeobjects and normalises falsy strings; duplicate select labels get(code)suffixes; failed sends roll back parameter mutations and command swaps.Confidence Score: 5/5
Safe to merge; all fixes are well-scoped, every changed path has a direct regression test, and no new failure modes were introduced.
The core invariants are preserved: command copies are properly isolated, the re-auth single-flight correctly collapses concurrent bursts on both success and failure paths, and entity-state fixes handle edge cases without regressing normal operation.
No files require special attention; the most complex change (HonCommand.copy + HonRuleSet.rebound) has thorough white-box tests in test_engine_cluster.py.
Important Files Changed
Reviews (5): Last reviewed commit: "test(diagnostics): cover MAC masking on ..." | Re-trigger Greptile