Refactor circuit discovery logic, enhance parameter validation

openspec/changes/archive/2026-07-04-harden-circuit-discovery-consistency/.openspec.yaml

@@ -0,0 +1,3 @@
+---
+schema: spec-driven
+created: 2026-07-04

openspec/changes/archive/2026-07-04-harden-circuit-discovery-consistency/design.md

@@ -0,0 +1,57 @@
+# Design: Harden Heating-Circuit Discovery Consistency
+
+## Context
+
+`get_available_circuits()` in [src/bsblan/bsblan.py](../../../src/bsblan/bsblan.py) short-circuits discovery with `self._json_api_version == MIN_SUPPORTED_JSON_API` — an exact string comparison against `"1.0"`. The basic single-circuit config (`API_BASIC`) is however selected by `VersionResolver.supports_full_config()` for the entire JSON-API range [1.0, 2.0), so a device reporting `"1.2"` bypasses the guard, probes circuit 2 via `CircuitConfig.PROBE_PARAMS`, and — because `API_BASIC` has an empty `heating_circuit2` section — a subsequent `state(circuit=2)` raises `BSBLANError` with `EMPTY_SECTION_PARAMS`.
+
+Independently, the "active parameter value" predicate `value not in (None, "---")` exists in three places: `APIValidator._is_valid_param` (utility.py), the probe loop in `get_available_circuits`, and `_get_available_pps_circuits`. `CircuitConfig.INACTIVE_MARKER = "---"` was introduced as the intended single source of that marker but was never wired in (dead constant). This duplication caused trigger B of the original bug (discovery accepted values that validation rejected).
+
+Constraints: no public API change, behavior-preserving except for the guard-range fix, coverage total ≥95% / patch 100%, `ty` type checking and Ruff via prek.
+
+## Goals / Non-Goals
+
+**Goals:**
+
+- Basic-config devices (any JSON-API version in [1.0, 2.0)) never probe circuit 2; discovery returns `[1]` and sets `_available_circuits = {1}`.
+- One shared predicate decides "param value is active" for validation and both discovery paths, sourced from `CircuitConfig.INACTIVE_MARKER`.
+
+**Non-Goals:**
+
+- `BSBLANCircuitNotAvailableError` (deferred; requires a paired home-assistant coordinator change).
+- Any change to version resolution (`VersionResolver`), config selection, or the PPS discovery flow's semantics.
+- Renaming/removing `MIN_SUPPORTED_JSON_API` (still used by `VersionResolver`).
+
+## Decisions
+
+### D1: Guard on `self._supports_full_config is False`, not version-string comparison
+
+The capability flag is the exact signal that selects `API_BASIC` in `_copy_api_config`, so guarding on it makes discovery agree with the config by construction — no parallel version-range logic to keep in sync. Alternatives considered:
+
+- *Parse and range-compare `_json_api_version`*: duplicates `VersionResolver` logic; drifts if thresholds change.
+- *Check `self._api_data` section emptiness per circuit* (`if not self._api_data.get(section): continue`): also correct and more granular, but `_api_data` may be `None` when `get_available_circuits()` is called before `initialize()` (a supported config-flow pattern), whereas the flag check degrades gracefully — `None`/`True` fall through to probing, preserving current behavior exactly.
+
+The `is False` identity check is deliberate: `_supports_full_config is None` (unresolved, e.g. discovery called standalone) must keep probing as today.
+
+### D2: Module-level `is_param_value_active(param)` in utility.py
+
+```python
+def is_param_value_active(param: dict[str, Any] | None) -> bool:
+    return bool(param) and param.get("value") not in (None, CircuitConfig.INACTIVE_MARKER)
+```
+
+- Placed in [src/bsblan/utility.py](../../../src/bsblan/utility.py) (module-level function, not a method) so both `APIValidator` and `BSBLAN` can import it without coupling the client to the validator class.
+- Sources `"---"` from `CircuitConfig.INACTIVE_MARKER`, reviving the dead constant as single source of truth. utility.py already imports from constants (`ErrorMsg`), so no import-cycle risk.
+- `APIValidator._is_valid_param` delegates to it (kept as a thin method — tests call it directly); both discovery loops in bsblan.py call the helper.
+- Truthiness contract is preserved: empty dict / `None` param → inactive; `value` of `None` or `"---"` → inactive; everything else active. Pure refactor.
+
+### D3: Test strategy
+
+- New `test_get_available_circuits_basic_config_skips_discovery` in tests/test_circuit.py: sets `_supports_full_config = False` with `_json_api_version = "1.2"`, asserts `[1]`, `{1}`, and `_request` never awaited — this fails on current code (falls through the guard).
+- Existing `test_get_available_circuits_json_api_v1_skips_discovery` updated to set `_supports_full_config = False` (as real initialization would for `"1.0"`), staying green.
+- `tests/test_utility.py::test_is_valid_param` keeps passing via the delegating method; add direct coverage of `is_param_value_active` (including the `None`-param branch) to hold patch coverage at 100%.
+
+## Risks / Trade-offs
+
+- [Guard now depends on `_supports_full_config` being resolved] → When unresolved (`None`), probing proceeds as before; a basic-config device probed pre-initialize would still hit the old path. Acceptable: identical to current behavior for that call order, and `initialize()`-first is the documented flow.
+- [Boolean-flag guard is coarser than per-section emptiness] → Fine today: `API_BASIC` is the only single-circuit config and the flag is its selector. If per-circuit configs ever diversify, revisit with section-emptiness checks.
+- [Formatter/hook churn on edits (known repo gotcha)] → Batch edits atomically; run `SKIP=no-commit-to-branch uv run prek run --all-files` after.

openspec/changes/archive/2026-07-04-harden-circuit-discovery-consistency/proposal.md

@@ -0,0 +1,31 @@
+# Harden Heating-Circuit Discovery Consistency
+
+## Why
+
+The circuit-discovery fix (plans/circuit-discovery-fix-plan.md, #1527/#1533) left two residual gaps. First, `get_available_circuits()` skips circuit probing only when the reported JSON-API version is exactly `"1.0"`, but the basic single-circuit config is selected for the whole range [1.0, 2.0) — a device reporting e.g. `"1.2"` still probes circuit 2 and can crash `state(circuit=2)` with `EMPTY_SECTION_PARAMS`. Second, the "parameter value is active" rule (`value not in (None, "---")`) is duplicated in three places while the intended constant `CircuitConfig.INACTIVE_MARKER` remains dead code, inviting future drift like trigger B of the original bug.
+
+## What Changes
+
+- Replace the exact-string guard `self._json_api_version == MIN_SUPPORTED_JSON_API` in `get_available_circuits()` with the capability signal that actually selects the basic config: `self._supports_full_config is False`. Behavior when `_supports_full_config` is `None` or `True` is unchanged; the basic-config path still returns `[1]` and sets `_available_circuits = {1}`.
+- Extract a single helper `is_param_value_active(param)` in `src/bsblan/utility.py`, sourcing the inactive marker from `CircuitConfig.INACTIVE_MARKER`, and reuse it at all three duplication sites: `APIValidator._is_valid_param`, `get_available_circuits`, and `_get_available_pps_circuits`. Pure refactor, no behavior change.
+- Add `test_get_available_circuits_basic_config_skips_discovery` covering a JSON-API version like `"1.2"` (currently falls through the guard); keep the existing `"1.0"` test green; adjust tests touching `_is_valid_param` internals as needed.
+
+Out of scope: `BSBLANCircuitNotAvailableError` (deferred; requires a paired home-assistant bsblan coordinator change).
+
+## Capabilities
+
+### New Capabilities
+
+- `circuit-discovery`: Heating-circuit discovery behavior — basic-config short-circuit driven by the resolved capability flag, probe-based discovery for full config, PPS single-circuit detection, and a single shared active-value predicate.
+
+### Modified Capabilities
+
+<!-- none — existing specs (client-retry-policy, cooling-operating-mode) are unaffected -->
+
+## Impact
+
+- `src/bsblan/bsblan.py`: `get_available_circuits()` guard condition; both discovery loops use the shared predicate.
+- `src/bsblan/utility.py`: new `is_param_value_active()` helper; `APIValidator._is_valid_param` delegates to it.
+- `src/bsblan/constants.py`: `CircuitConfig.INACTIVE_MARKER` becomes the single source of the `"---"` marker (no value change).
+- `tests/test_circuit.py`, `tests/test_utility.py`: new basic-config discovery test; existing `"1.0"` test updated to set the capability flag it now depends on.
+- No public API change; coverage gates unchanged (total ≥95%, patch 100%).

openspec/changes/archive/2026-07-04-harden-circuit-discovery-consistency/specs/circuit-discovery/spec.md

@@ -0,0 +1,46 @@
+# Circuit Discovery
+
+## ADDED Requirements
+
+### Requirement: Basic-config devices skip circuit probing
+
+When the resolved API capability is the basic single-circuit configuration (`_supports_full_config is False`), `get_available_circuits()` SHALL NOT probe any circuit parameters and SHALL report exactly circuit 1 (return `[1]` and set `_available_circuits = {1}`). The guard MUST be driven by the capability flag that selects the basic configuration, not by comparing the reported JSON-API version string.
+
+#### Scenario: JSON-API version 1.2 (basic-config range) skips discovery
+
+- **WHEN** `_supports_full_config` is `False` and `_json_api_version` is `"1.2"` and `get_available_circuits()` is called
+- **THEN** no parameter requests are made, the method returns `[1]`, and `_available_circuits` is `{1}`
+
+#### Scenario: JSON-API version 1.0 keeps skipping discovery
+
+- **WHEN** `_supports_full_config` is `False` and `_json_api_version` is `"1.0"` and `get_available_circuits()` is called
+- **THEN** no parameter requests are made, the method returns `[1]`, and `_available_circuits` is `{1}`
+
+#### Scenario: Full-config device probes circuits unchanged
+
+- **WHEN** `_supports_full_config` is `True` and `get_available_circuits()` is called
+- **THEN** each circuit in `CircuitConfig.PROBE_PARAMS` is probed and circuits with an active operating-mode value are reported
+
+#### Scenario: Unresolved capability probes circuits unchanged
+
+- **WHEN** `_supports_full_config` is `None` (discovery called before capability resolution) and `get_available_circuits()` is called
+- **THEN** discovery probes circuits exactly as it does today (no short-circuit)
+
+### Requirement: Single shared active-value predicate
+
+The library SHALL provide a single helper `is_param_value_active(param)` in `utility.py` that determines whether a parameter payload carries an active value: the payload MUST be non-empty and its `value` MUST NOT be `None` or the inactive marker sourced from `CircuitConfig.INACTIVE_MARKER`. `APIValidator._is_valid_param`, the heating-circuit probe loop in `get_available_circuits`, and `_get_available_pps_circuits` SHALL all use this helper. This is a behavior-preserving refactor.
+
+#### Scenario: Active value accepted
+
+- **WHEN** `is_param_value_active({"value": "1", "unit": "", "desc": "Automatic"})` is evaluated
+- **THEN** it returns `True`
+
+#### Scenario: Inactive marker rejected
+
+- **WHEN** the parameter payload has `value` equal to `"---"` or `None`, or the payload is empty/`None`
+- **THEN** `is_param_value_active` returns `False`
+
+#### Scenario: Validation and discovery agree
+
+- **WHEN** a probe parameter response would be rejected by `APIValidator._is_valid_param`
+- **THEN** circuit discovery rejects the same response (both paths delegate to `is_param_value_active`)

openspec/changes/archive/2026-07-04-harden-circuit-discovery-consistency/tasks.md

@@ -0,0 +1,26 @@
+# Tasks: Harden Heating-Circuit Discovery Consistency
+
+## 1. Shared active-value predicate (pure refactor)
+
+- [x] 1.1 Add module-level `is_param_value_active(param: dict[str, Any] | None) -> bool` to `src/bsblan/utility.py`, sourcing the inactive marker from `CircuitConfig.INACTIVE_MARKER` (import from `.constants`); semantics: non-empty payload AND `value` not in `(None, INACTIVE_MARKER)`
+- [x] 1.2 Delegate `APIValidator._is_valid_param` to `is_param_value_active` (keep the method — tests call it directly)
+- [x] 1.3 In `src/bsblan/bsblan.py` `get_available_circuits()` probe loop, replace `if not param_data or param_data.get("value") in (None, "---"):` with `if not is_param_value_active(param_data):` (import helper from `.utility`)
+- [x] 1.4 In `_get_available_pps_circuits()`, replace the two-step empty-dict + `value in (None, "---")` checks with a single `is_param_value_active` check (preserve the debug-log behavior for the reject path)
+
+## 2. Basic-config discovery guard
+
+- [x] 2.1 In `get_available_circuits()`, replace `if self._json_api_version == MIN_SUPPORTED_JSON_API:` with `if self._supports_full_config is False:` (keep returning `[1]` and setting `self._available_circuits = {1}`); update the debug log message to reference basic configuration instead of "JSON-API version 1.0"
+- [x] 2.2 Remove the now-unused `MIN_SUPPORTED_JSON_API` import from `bsblan.py` if no other usage remains (it stays in `_version.py`/`constants.py`)
+
+## 3. Tests
+
+- [x] 3.1 Add `test_get_available_circuits_basic_config_skips_discovery` in `tests/test_circuit.py`: set `_supports_full_config = False` and `_json_api_version = "1.2"`, assert return `[1]`, `_available_circuits == {1}`, and `_request` not awaited (note: `mock_bsblan_circuit` fixture sets `_supports_full_config = True`, so override after fixture)
+- [x] 3.2 Update existing `test_get_available_circuits_json_api_v1_skips_discovery` to set `_supports_full_config = False` (mirrors real resolution for "1.0"); keep its assertions green
+- [x] 3.3 Verify full-config and unresolved-capability paths still probe: confirm existing probe tests pass with `_supports_full_config = True` from the fixture; add a `None`-capability probe test only if not already covered
+- [x] 3.4 In `tests/test_utility.py`, keep `test_is_valid_param` green via delegation and add direct tests for `is_param_value_active` (active value, `"---"`, `None` value, empty dict, `None` payload) for 100% patch coverage
+
+## 4. Validation
+
+- [x] 4.1 Run `uv run pytest --no-cov tests/test_circuit.py tests/test_utility.py`
+- [x] 4.2 Run `SKIP=no-commit-to-branch uv run prek run --all-files` and confirm green (expect one possible formatter pass)
+- [x] 4.3 Confirm coverage gates: total ≥95%, patch 100% (`uv run pytest --cov=src/bsblan --cov-report=term-missing`)

openspec/specs/circuit-discovery/spec.md

@@ -0,0 +1,77 @@
+# circuit-discovery
+
+## Purpose
+
+Heating-circuit discovery behavior of the BSB-LAN client: how
+`get_available_circuits()` decides which circuits exist, how the basic
+single-circuit configuration short-circuits probing, and the single shared
+predicate that decides whether a parameter value is active — so discovery and
+section validation never disagree.
+
+## Requirements
+
+### Requirement: Basic-config devices skip circuit probing
+
+When the resolved API capability is the basic single-circuit configuration
+(`_supports_full_config is False`), `get_available_circuits()` SHALL NOT probe
+any circuit parameters and SHALL report exactly circuit 1 (return `[1]` and
+set `_available_circuits = {1}`). The guard MUST be driven by the capability
+flag that selects the basic configuration, not by comparing the reported
+JSON-API version string.
+
+#### Scenario: JSON-API version 1.2 (basic-config range) skips discovery
+
+- **WHEN** `_supports_full_config` is `False` and `_json_api_version` is
+  `"1.2"` and `get_available_circuits()` is called
+- **THEN** no parameter requests are made, the method returns `[1]`, and
+  `_available_circuits` is `{1}`
+
+#### Scenario: JSON-API version 1.0 keeps skipping discovery
+
+- **WHEN** `_supports_full_config` is `False` and `_json_api_version` is
+  `"1.0"` and `get_available_circuits()` is called
+- **THEN** no parameter requests are made, the method returns `[1]`, and
+  `_available_circuits` is `{1}`
+
+#### Scenario: Full-config device probes circuits unchanged
+
+- **WHEN** `_supports_full_config` is `True` and `get_available_circuits()`
+  is called
+- **THEN** each circuit in `CircuitConfig.PROBE_PARAMS` is probed and
+  circuits with an active operating-mode value are reported
+
+#### Scenario: Unresolved capability probes circuits unchanged
+
+- **WHEN** `_supports_full_config` is `None` (discovery called before
+  capability resolution) and `get_available_circuits()` is called
+- **THEN** discovery probes circuits exactly as it does today (no
+  short-circuit)
+
+### Requirement: Single shared active-value predicate
+
+The library SHALL provide a single helper `is_param_value_active(param)` in
+`utility.py` that determines whether a parameter payload carries an active
+value: the payload MUST be non-empty and its `value` MUST NOT be `None` or
+the inactive marker sourced from `CircuitConfig.INACTIVE_MARKER`.
+`APIValidator._is_valid_param`, the heating-circuit probe loop in
+`get_available_circuits`, and `_get_available_pps_circuits` SHALL all use
+this helper.
+
+#### Scenario: Active value accepted
+
+- **WHEN** `is_param_value_active({"value": "1", "unit": "", "desc":
+  "Automatic"})` is evaluated
+- **THEN** it returns `True`
+
+#### Scenario: Inactive marker rejected
+
+- **WHEN** the parameter payload has `value` equal to `"---"` or `None`, or
+  the payload is empty/`None`
+- **THEN** `is_param_value_active` returns `False`
+
+#### Scenario: Validation and discovery agree
+
+- **WHEN** a probe parameter response would be rejected by
+  `APIValidator._is_valid_param`
+- **THEN** circuit discovery rejects the same response (both paths delegate
+  to `is_param_value_active`)

src/bsblan/bsblan.py

@@ -23,7 +23,6 @@
 from .constants import (
     API_BASIC,
     API_FULL,
-    MIN_SUPPORTED_JSON_API,
     APIConfig,
     CircuitConfig,
     ErrorMsg,
@@ -51,7 +50,7 @@
     State,
     StaticState,
 )
-from .utility import validate_time_format
+from .utility import is_param_value_active, validate_time_format
 
 if TYPE_CHECKING:
     from typing import Self
@@ -216,10 +215,10 @@ async def get_available_circuits(self) -> list[int]:
                 # circuits == [1, 2] for a dual-circuit controller
 
         """
-        if self._json_api_version == MIN_SUPPORTED_JSON_API:
+        if self._supports_full_config is False:
             logger.debug(
-                "BSBLAN JSON-API version 1.0 detected; skipping circuit discovery and "
-                "assuming only circuit 1 is available"
+                "Basic single-circuit configuration detected; skipping circuit "
+                "discovery and assuming only circuit 1 is available"
             )
             self._available_circuits = {1}
             return [1]
@@ -240,9 +239,9 @@ async def get_available_circuits(self) -> list[int]:
                 continue
 
             # A circuit exists if the response contains the operating mode key
-            # with a valid value (not an empty dict, and not None/"---").
+            # with an active value (not an empty dict, and not None/"---").
             param_data = response.get(param_id)
-            if not param_data or param_data.get("value") in (None, "---"):
+            if not is_param_value_active(param_data):
                 logger.debug(
                     "Circuit %d has no operating mode data (not supported)",
                     circuit,
@@ -263,13 +262,8 @@ async def _get_available_pps_circuits(self) -> list[int]:
             self._available_circuits = set()
             return []
 
-        if not response.get(param_id):
-            logger.debug("PPS climate circuit has no operating mode data")
-            self._available_circuits = set()
-            return []
-        param_data = response[param_id]
-        if param_data.get("value") in (None, "---"):
-            logger.debug("PPS climate circuit has invalid operating mode value")
+        if not is_param_value_active(response.get(param_id)):
+            logger.debug("PPS climate circuit has no active operating mode value")
             self._available_circuits = set()
             return []
         self._available_circuits = {1}