fix: dosing setpoint validation gaps, retry-bypassed rate limiter, decimal parsing

[Xerolux]

Summary

Full-codebase review (correctness + optimization) of violet_poolcontroller_api, covering api.py, circuit_breaker.py, utils_rate_limiter.py, readings.py, _api_readings.py, _api_outputs.py, _api_dosing.py, _api_system.py, and utils_sanitizer.py. pytest, mypy, ruff, and bandit were all clean before this change; the findings below came from a manual line-by-line review plus verification against the mock server fixtures. Each fix below is backed by a new regression test.

• set_dosing_parameters() bypassed setpoint safety validation — it forwarded the caller's dict straight to set_config(), skipping the pH/ORP/min-chlorine range checks that set_ph_target()/set_orp_target()/set_min_chlorine_level() enforce for the exact same keys. A caller could push a pH setpoint of 20.0 straight to the dosing controller. Now validates any known setpoint key (SETPOINT_RANGES) present in the payload before sending.
• Rate limiter was bypassed on every retry — wait_if_needed() was awaited once before the retry loop in _execute_request(), so each of the (up to max_retries) real HTTP attempts inside the loop skipped the limiter entirely. Moved the wait inside the loop so every attempt re-acquires a token.
• dosing_daily_amounts_ml / get_system_services() dropped decimal values — both used int() directly on strings the controller actually sends in decimal form ("12.5", "1.0"), which raises/crashes instead of parsing. Confirmed against the project's own mock fixtures (tests/mock_server.py). Both now go through float() first; get_system_services() now raises VioletPayloadError (matching the existing pattern in is_dosage_enabled()) instead of an unhandled ValueError for genuinely invalid states.
• InputSanitizer.sanitize_numeric() mangled scientific notation — it stripped e/+/- characters instead of parsing them, so "1e10" silently became 110.0 (off by ~9 orders of magnitude) with no error logged. Now attempts a direct float() parse first.
• InputSanitizer.validate_ph_value() used the wrong range — allowed pH up to 9.0 while the controller's actual setpoint bound (SETPOINT_RANGES[TARGET_PH], enforced by set_ph_target()) is 8.0. A value like 8.5 would pass this validator and then be rejected by the setter. Aligned to 6.0–8.0.
• set_output_test_mode() sent an un-encoded payload — inconsistent with the identical set_switch_state() pattern, which URL-encodes via quote(payload, safe=","). Fixed for consistency/robustness against reserved characters.
• set_can_amount() truncation vs. validation mismatch — amount_ml in (0, 1) passed the > 0 check but was then truncated to 0 by int() before being sent, silently violating its own documented contract.
• set_rs485_live() raised the wrong exception type — a non-numeric slave_id/level raised a bare TypeError from float()/int() instead of the documented ValueError.
• 5xx/429 responses didn't drain the body before raise_for_status() — forced aiohttp to close the connection instead of returning it to the pool, adding a fresh TCP/TLS handshake to every retry during exactly the period the controller is already degraded.

Not changed (reviewed, judged out of scope / too speculative for this pass)

• Legacy numeric COVER_STATE encoding (COVER_STATE_MAP exists but isn't wired into the typed parser) — no confirmed evidence current firmware emits it.
• _dosing_standalone cross-call mutable state under concurrent asyncio.gather — narrow/latent, would need a wider refactor.
• get_hardware_profile() re-fetching the full getReadings payload — efficiency-only, no correctness impact.
• Retry-After header only supporting numeric seconds (not HTTP-date form) — graceful degradation already in place.

Test plan

• pytest — 221 passed (was 211; added targeted regression tests for each fix)
• mypy violet_poolcontroller_api/ — clean
• ruff check — clean
• bandit -r violet_poolcontroller_api/ — no issues

---

Generated by Claude Code

Summary by Sourcery

Tighten dosing and system API validation and rate limiting behavior to match controller contracts and avoid silent failures.

Bug Fixes:

• Enforce setpoint range validation in set_dosing_parameters and normalize setpoint numeric parsing to raise VioletSetpointError on invalid input.
• Fix rate limiter so each HTTP retry attempt re-acquires a token and ensure 5xx/429 responses drain their bodies before raising.
• Parse decimal strings correctly for dosing_daily_amounts_ml and get_system_services, raising VioletPayloadError on invalid service states instead of bare ValueError.
• Correct InputSanitizer numeric handling to support scientific notation and align pH validation bounds with the controller’s 6.0–8.0 setpoint range.
• Ensure set_output_test_mode URL-encodes its payload, prevent set_can_amount from accepting values that truncate to zero, and make set_rs485_live raise documented ValueError types for bad inputs.

Tests:

• Add targeted regression tests covering dosing setpoint range enforcement, decimal parsing of dosing amounts and system services, rate limiter behavior on retries, numeric sanitizer scientific-notation support, and pH bound alignment.

[sourcery-ai]

Reviewer's Guide

Strengthens safety and correctness across dosing, system, and HTTP layers by enforcing setpoint validation paths, fixing decimal/scientific numeric parsing, ensuring the rate limiter and connection reuse behave correctly on retries, and adding regression tests for each fix.

File-Level Changes

Change	Details	Files
Harden dosing setpoint APIs to validate ranges consistently and normalize numeric input errors.	Introduce _to_float helper to convert setpoint values to float and wrap type/parse errors in VioletSetpointError. Update set_ph_target, set_orp_target, set_min_chlorine_level, and set_target_value to use _to_float and pass numeric values into validate_setpoint. Make set_dosing_parameters pre-validate any keys present in SETPOINT_RANGES using validate_setpoint and _to_float before forwarding to set_config. Tighten set_can_amount to validate amount_ml via its integer value, rejecting positive fractional values that truncate to zero.	violet_poolcontroller_api/_api_dosing.py
Fix dosing and system numeric parsing to support decimals and invalid-data behavior without crashes.	Change VioletReadings.dosing_daily_amounts_ml to parse wire values via float() before int(), treating non-numeric/missing values as None. Update get_system_services to parse service state strings via int(float(...)) and raise VioletPayloadError on invalid values, while still mapping numeric decimals to booleans. Add regression tests covering decimal dosing amounts, missing/garbage dosing values, decimal service states, and invalid service states.	violet_poolcontroller_api/readings.py violet_poolcontroller_api/_api_system.py tests/test_readings.py tests/test_api.py
Ensure the HTTP rate limiter and error handling apply per-attempt and preserve connection reuse under failures.	Move rate limiter wait_if_needed invocation inside the retry loop in _request._execute_request so every attempt re-acquires a token and still applies timeout fallback sleep. On 5xx and 429 responses, read the entire response body before raise_for_status so aiohttp can recycle the connection instead of closing it. Add an async regression test to assert that wait_if_needed is called once per retry attempt when repeated failures occur.	violet_poolcontroller_api/api.py tests/test_api.py
Improve numeric input sanitization and pH validation to match controller constraints and support scientific notation.	Make InputSanitizer.sanitize_numeric first try float() on the stripped string and return parsed finite values, logging and zeroing infinities/NaN, then fall back to the old digit-extraction behavior for messy strings. Align validate_ph_value to a 6.0–8.0 range (instead of 6.0–9.0) with unchanged default and precision, matching the controller’s SETPOINT_RANGES. Add regression tests for scientific-notation parsing, messy numeric strings, and pH bound behavior.	violet_poolcontroller_api/utils_sanitizer.py tests/test_sanitizer.py
Tighten output/RS485 control APIs for encoding and error handling consistency.	Update set_output_test_mode to URL-encode its query payload via quote(..., safe=",") before issuing the request, matching set_switch_state behavior. Change set_rs485_live to normalize slave_id to an int with explicit ValueError on non-numeric input, validate it in the 1–247 range, and explicitly validate level as a finite number while raising ValueError on non-numeric values. Keep URL generation for set_rs485_live consistent by using the normalized slave_id_int while leaving level as passed through after validation. Add or rely on tests around RS485 validation where appropriate.	violet_poolcontroller_api/_api_outputs.py
Extend API tests to cover new error contracts and behaviors around dosing and services.	Import VioletSetpointError and TARGET_PH into tests to assert dosing setpoint safety behavior. Add tests that set_dosing_parameters enforces setpoint range and raises VioletSetpointError on out-of-range pH values. Add tests that get_system_services accepts decimal states, maps them to booleans, and raises VioletPoolAPIError (from VioletPayloadError) for garbage states.	tests/test_api.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• In set_target_value you compute numeric_value = _to_float(key, value) but still pass the original value through to set_config; consider sending numeric_value instead so the payload is consistently normalized and matches the value you just validated.
• The new get_system_services parsing uses bool(int(float(raw_value))), which will silently treat any non-zero numeric (e.g. '0.5') as enabled; if the protocol only allows 0/1 or 0.0/1.0, you may want to assert on those specific values and raise for anything else rather than coercing arbitrary numerics.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `set_target_value` you compute `numeric_value = _to_float(key, value)` but still pass the original `value` through to `set_config`; consider sending `numeric_value` instead so the payload is consistently normalized and matches the value you just validated.
- The new `get_system_services` parsing uses `bool(int(float(raw_value)))`, which will silently treat any non-zero numeric (e.g. '0.5') as enabled; if the protocol only allows 0/1 or 0.0/1.0, you may want to assert on those specific values and raise for anything else rather than coercing arbitrary numerics.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}