Skip to content

chore: bump version to 2.0.0.B4 - #167

Merged
KJonline merged 3 commits into
devfrom
fix/ha-compat-review
Oct 15, 2026
Merged

KJonline merged 3 commits into
devfrom
fix/ha-compat-review

Conversation

@KJonline

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/session/auth.py Outdated
HiveInvalidUsername,
HiveInvalidPassword,
),
retry_on=(HiveApiError,),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Claude Code Review

retry_on=(HiveApiError,) changes the failure contract of _retry_login. Before, any exception from login() was retried 3 times and then became HiveReauthRequired. Now anything that isn't a HiveApiError escapes on the first attempt, with no retry and no conversion. That includes HiveInvalidDeviceAuthentication (saved device credentials rejected, which is what needs a re-login), raw botocore/aiohttp errors from the Cognito calls, and KeyError/TypeError from malformed responses.

The CLAUDE.md contract says HiveReauthRequired is the only exception application code has to handle. Home Assistant maps that exception to ConfigEntryAuthFailed (reauth flow). With this change, a rejected device login or a transient network error during refresh surfaces as an unhandled exception, so setup fails with a generic error instead of a reauth prompt or a retry.

Suggested fix: keep retrying and converting these cases. For example, add HiveInvalidDeviceAuthentication to retry_on or to a conversion to HiveReauthRequired. Alternatively, wrap transport errors in HiveConnectionError at the login() boundary so HiveApiError covers them.

@claude

claude Bot commented Oct 15, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Verdict: mostly good, but one behaviour regression in _retry_login should be fixed before merge.

Bugs

  • src/session/auth.py: retry_on=(HiveApiError,) means non-HiveApiError failures (HiveInvalidDeviceAuthentication, raw botocore/aiohttp errors, malformed-response KeyError) are no longer retried or converted to HiveReauthRequired. They propagate raw to Home Assistant. See the inline comment.

Should fix

  • None.

Minor

  • color.py get_color truncates with int(channel * 255). round() would give more accurate RGB (e.g. 0.8 → 204 happens to work, but other values lose 1 step). color_RGB_to_hs is tolerant, so this is low impact.
  • start_session does config.get("options", {}). This would raise AttributeError if options is explicitly None. Unlikely from HA.

Home Assistant check

  • hive/light.py: rgb = self.device["status"]["hs_color"] followed by color_util.color_RGB_to_hs(*rgb). The switch to an RGB 3-tuple in get_color is correct.
  • hive/__init__.py: await hive.session.startSession(hive_config) with hive_config = dict(entry.data). The fetched file does not read options.scan_interval, so the new options handling in start_session is not exercised by that file today. It is harmless.
  • I did not fetch the other HA files, so the camelCase alias additions are unchecked against HA callers. The PR says they serve the custom component.

Not reviewed

  • The generated sync package, and a full test and coverage run.

Automated review by Claude Code · workflow run

@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (c044382) to head (ef577d7).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #167   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files          30       30           
  Lines        2599     2643   +44     
  Branches      306      310    +4     
=======================================
+ Hits         2598     2642   +44     
  Partials        1        1           
Flag Coverage Δ
unittests 99.96% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/devices/color.py 100.00% <100.00%> (ø)
src/helper/compat_aliases.py 100.00% <100.00%> (ø)
src/session/auth.py 100.00% <100.00%> (ø)
src/session/discovery.py 100.00% <100.00%> (ø)
src/session/polling.py 99.42% <100.00%> (+0.02%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…dle options=None

Add HiveInvalidDeviceAuthentication to the set of exceptions that trigger
HiveReauthRequired in _retry_login, since rejected device credentials need
a fresh SMS login. Replace config.get("options", {}) with
(config.get("options") or {}) so options=None doesn't raise AttributeError.
Add tests for both cases.
@claude

claude Bot commented Oct 15, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Verdict: no new bugs found. The earlier retry_on=(HiveApiError,) concern in _retry_login (non-HiveApiError failures propagate raw instead of becoming HiveReauthRequired) still stands and is not repeated here.

Bugs

  • None new.

Should fix

  • _retry_login retry_on narrowing: see the earlier review comment.

Minor

  • get_color uses int(channel * 255). round() would be more accurate.
  • update_interval silently raises values below 15s to 15s. Worth a debug log so callers know.

Home Assistant check

  • I did not fetch Home Assistant files in this run; I relied on the earlier review's check of hive/light.py (color_RGB_to_hs(*rgb)) and hive/__init__.py. The updateInterval alias and the camelCase additions are unchanged in signature for existing callers.

Not reviewed

  • Full test and coverage run, and the generated sync package.

Automated review by Claude Code · workflow run

Change int() to round() in LightColorHandler.get_color so HSV->RGB conversion rounds channels instead of truncating (2.55 -> 3, not 2). Remove retry_on parameter from _retry_with_delays since it always retried Exception anyway. Retry every login failure (network, malformed response, library errors) in _retry_login instead of only HiveApiError, converting all exhausted retries to HiveReauthRequired. Add tests for channel rounding and transient failure
@claude

claude Bot commented Oct 15, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Verdict: no blocking issues found. The earlier _retry_login concern is resolved in this head.

Bugs

  • None found. _retry_with_backoff retries every non-pass-through Exception and then raises HiveReauthRequired. HiveInvalidDeviceAuthentication now maps to HiveReauthRequired. options=None is handled by (config.get("options") or {}). get_color now rounds the RGB channels.

Minor

  • update_interval (and the updateInterval alias) now clamps values below 15s to 15s. Before, any value was accepted. This is intentional per the docstring, but it is a silent behaviour change for callers that pass a smaller interval.
  • _parse_token_created returns a naive datetime when given an ISO string. That matches the naive datetime.now() used elsewhere. If a saved string ever includes a timezone offset, the token_created + token_expiry comparison with datetime.now() would raise TypeError. That is unlikely given how the value is produced.

Home Assistant check

  • Not fetched in this run. I relied on the earlier review's note that HA's light.py calls color_util.color_RGB_to_hs(*rgb) on status["hs_color"], which matches the RGB 3-tuple. I did not check the new camelCase aliases or updateInterval against HA callers.

Not reviewed

  • The generated sync package and a full test and coverage run. Codecov reports the changed lines as covered.

Automated review by Claude Code · workflow run

@KJonline
KJonline merged commit 2979710 into dev Oct 15, 2026
13 checks passed
@KJonline
KJonline deleted the fix/ha-compat-review branch October 15, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant