feat(group): target-aware scene status from /api/groups link data

[shbatm]

Implements the first slice of #174. Composes with #172 (merged) — that
delivers the change notification (member→group re-emit); this makes
the value correct.

Problem

group_any_on / group_all_on were a coarse "any/all stateful member
ST non-zero" aggregate. For a radio-style KeypadLinc scene a keypad
always has exactly one button lit, so group_any_on was permanently
True and group_all_on structurally impossible — the scene switch
never reflected reality.

Fix

Fetch the optional /api/groups endpoint during load and resolve each
member's scene intent from the canonical ctl block (the one whose
id == the group address — the scene's own responder definition;
per-controller cross-link blocks are ignored):

link type	intent
native	OL > 0 → on, OL == 0 → off
default	on (ISY forwards the scene command; state-tracked)
cmd	DON/DFON → on, DOF/DFOF → off, anything else (BL/BEEP/SETST/BRT/DIM/FD*) → discard
ignore	not a member
unknown / native w/o OL	unresolved → legacy aggregate (never regress)

group_any_on/group_all_on now aggregate over the on-target
subset. An empty on-set (fire-only / config-only / the auto-DR groups)
reads OFF, matching the IoX admin console. Group.has_state_target
exposes whether the scene maintains any on/off state — False for a
fire-only scene, so a consumer can model it as a momentary button
rather than a stuck switch (hacs-udi-iox#86).

Strictly additive and consumer-transparent: /api/groups is
best-effort (older firmware 404 → {} → groups keep legacy behaviour);
exact on-level match (scene_matches_target) is deliberately deferred.

Validated against real captures: radio scene 8499 (one OL=100
member, rest OL=0), curtain 19359, the auto-DR ADR0001 (empty
canonical block → OFF), and the default+cmd/BL-only test scenes.

Changes

• paths: GROUPS_PATH
• client: _get_json_or_empty; GroupRecord.member_intents +
  targets_resolved; apply_group_link_targets(); wired into load()
  (so refresh() re-enriches too)
• runtime/group: _on_set(), has_state_target, target-aware
  group_any_on/group_all_on with legacy fallback; to_dict adds
  has_state_target
• tests: 18 parse + aggregation cases; FakeSession defaults
  /api/groups to an empty payload so the many full-load tests need no
  per-test scripting; full-load fan-out asserted 8 → 9

Scene HA-platform inputs (has_state_target + has_dimmable_members)

pyisyox exposes two orthogonal capability booleans; the consumer
composes the platform (capability stays here, the HA light/switch/button
decision stays policy):

• Group.has_state_target — False only for a resolved fire-only /
  config scene (→ button); True otherwise (incl. unresolved:
  conservative, stays a switch).
• Group.has_dimmable_members — True iff any member node
  is_dimmable (nodedef-derived; independent of /api/groups so it
  works on older firmware / unresolved groups too).

Consumer mapping: not has_state_target → button; else
has_dimmable_members → on/off light (lets hacs create the scene
natively at the right platform, preserving the Entity.group /
more-info framework instead of losing it through switch_as_x); else →
switch. Scenes have no settable brightness — ISY groups don't take a
scene-level on-level; fade/brt/dim are separate manual commands
(reachable via send_node_command; scene accepts aren't exposed as
buttons yet) — so the light is on/off only.

Review nits from the first pass are addressed (rank > first-seen,
""-sentinel comment, docstring, +no-ctl / reverse-precedence /
all-stateless / has_dimmable tests).

867 passed · mypy clean · pylint 10/10 · ruff clean.

🤖 Generated with Claude Code

[github-actions]

Claude finished @shbatm's task in 3m 34s —— View job

---

Code Review: PR #175 — feat(group): target-aware scene status from /api/groups link data

Tasks:

• Gather context and understand the PR
• Review pyisyox/paths.py changes
• Review pyisyox/client.py changes
• Review pyisyox/runtime/group.py changes
• Review test changes
• Compile and post final review

---

Summary

This is a well-designed additive feature. The intent-resolution logic is cleanly separated, the legacy fallback is solid (targets_resolved=False → unchanged behaviour), and the test suite covers the stated scenarios well (18 new parse + aggregation cases, radio scene, fire-only/auto-DR, precedence, legacy fallback). A few minor observations below — none are blockers.

---

pyisyox/client.py

_link_intent — "" sentinel is subtle but handled correctly

The function returns three distinct signals: "on"/"off"/"discard" (resolved intent), "" (ignore-type, not a member), and None (ambiguous, abort resolution). The "" sentinel is a slightly unusual interface — two different None-like meanings are split across None and "". The calling code in apply_group_link_targets handles it correctly:

if not node or intent == "":
    continue  # ignore-link / malformed

But it's easy for a future reader to conflate the None path (abort entire group) with the "" path (skip this one link). A short comment on the return "" line in _link_intent noting "skip silently, but don't abort resolution" would make the intent unambiguous.

apply_group_link_targets — rank comparison uses >=

client.py:1181:

if rank >= ranks.get(node, -1):
    intents[node] = intent
    ranks[node] = rank

>= means a later same-type link for the same node overwrites an earlier one (last-seen wins within a type). For distinct types this doesn't matter (native=3 always beats default=1). For same-type duplicate entries (e.g. two native links for the same node in one canonical block) the second one wins silently. In practice this would indicate malformed /api/groups data, but changing to > would give first-seen-wins semantics which is usually safer when encountering unexpected duplicates. Minor either way.

apply_group_link_targets — missing-canonical-block edge case not tested

When a group entry appears in /api/groups but has no ctl array (or an empty ctl array), the loop body never executes, resolved stays True, and intents stays {}. The group is committed as targets_resolved=True with member_intents={} — effectively a fire-only/empty scene. This is correct, but the only test covering this path is test_apply_targets_empty_canonical_block_resolves_empty (empty links). A companion test for the case where ctl itself is absent or empty would round out the coverage:

def test_apply_targets_no_ctl_block_resolves_empty():
    groups = {"G": _grec("G", ("X",))}
    apply_group_link_targets(groups, _api_groups({"id": "G"}))  # no "ctl" key
    assert groups["G"].targets_resolved is True
    assert groups["G"].member_intents == {}

_get_json_or_empty swallows all HTTPError, not just 404

client.py:650–658: Consistent with the existing _get_text_or_empty pattern for networking resources, so this is intentional. The docstring says "a 404 falls back to {}" but the implementation swallows any HTTP error (401, 500, etc.). Not a regression — the networking helper does the same — but the docstring is slightly misleading. Consider: "any HTTP error falls back to {}" to match actual behaviour.

---

pyisyox/runtime/group.py

group_all_on — stateless nodes in the on-set cause silent False

group.py:221–231: When on_set is non-empty but every address in it happens to be stateless (excluded by _is_stateless), saw_stateful remains False and the property returns False rather than the more accurate "unknown". This pre-existed for the legacy all-member path. In practice, /api/groups won't list stateless nodes (motion sensors, RemoteLincs) as on-target responders, so this is a theoretical edge case. No action needed, just worth noting for completeness.

group_any_on vs group_all_on — asymmetric handling of missing registry entries

Pre-existing, not introduced here: in group_all_on a missing node causes immediate False; in group_any_on a missing node is silently skipped. Both apply consistently to both the on-set and legacy paths, so the PR doesn't regress anything. Flagging for awareness.

has_state_target default (True when unresolved) is the right safe choice

Confirmed correct: unresolved groups have targets_resolved=False, so has_state_target returns True (conservative — keeps the scene as stateful). Resolved fire-only scenes return False when member_intents has no "on" or "off" entries. The test test_group_has_state_target_true_when_off_intent_present explicitly validates the "off"-only case is still considered stateful.

---

Tests

tests/test_runtime/test_groups.py

Coverage is good. A couple of gaps worth considering:

1. No test for group_all_on with a resolved group whose entire on-set consists of stateless nodes (theoretical, but documents the silent-False behaviour noted above).

2. No test for precedence in reverse insertion order: native appearing before default for the same node. The existing test test_apply_targets_native_beats_default_for_same_node puts default first then native. Both orderings are correct (3 ≥ 1 and 1 ≥ 3 behaves differently), so it's worth a companion to lock in the reverse case. Fix this →

3. The FakeSession default for /api/groups is a clean ergonomic improvement — avoids boilerplate in every existing load test.

---

pyisyox/paths.py

Clean, well-documented addition. No issues.

---

Overall

Design is sound: best-effort enrichment, conservative fallback to legacy aggregate, has_state_target gives consumers the right primitive to decide button-vs-switch modelling. The targets_resolved flag cleanly separates the "we know" from the "we don't know" cases. Ready to merge pending the minor items above (all non-blocking).

[github-actions]

Claude finished @shbatm's task in 2m 44s —— View job

---

Code Review: PR #175 — feat(group): target-aware scene status from /api/groups link data

Tasks:

• Gather context and understand the PR
• Review updated changes (addressing nits + has_dimmable_members)
• Review pyisyox/client.py changes
• Review pyisyox/runtime/group.py changes
• Review test changes
• Compile and post final review

---

Summary

The follow-up commit (4582da2) addresses all five items raised in the previous review and adds a new has_dimmable_members property. Everything is solid — no blockers.

---

Previous Review Items — All Addressed ✓

Previous comment	Resolution
>= → > for same-type duplicate links	Fixed: strict > in apply_group_link_targets — first-seen wins for same-type duplicates
Reverse insertion order (native before default) not tested	Added test_apply_targets_native_beats_default_reverse_order
Missing test for absent ctl key in group entry	Added test_apply_targets_no_ctl_block_resolves_empty
_get_json_or_empty docstring said "404" only	Updated: docstring now says "404, 401, 500, …"
Stateless-only on-set causing silent False undocumented	Added test_group_all_on_resolved_on_set_all_stateless_is_false

---

New Feature: has_dimmable_members (group.py:196–224)

Design is correct and consistent with the CLAUDE.md capability-vs-policy boundary. The property is nodedef-derived (capability → pyisyox), independent of link-target resolution, so it degrades gracefully on older firmware. The docstring is unusually thorough and gives consumers a clear decision tree (no-state → button; dimmable → light; else → switch).

One minor observation: has_dimmable_members constructs a temporary Node object per member via Node.from_record(record, self._profile, self._client) on every call, solely to reach is_dimmable. Each call does a profile.find_nodedef + profile.find_editor dict lookup — cheap for typical scene sizes (< 50 members), but called unbounded on every to_dict(). For the current use-cases this is fine. If it's ever called in a hot loop over thousands of groups, a lazy _has_dimmable: bool | None cache could be added later.

Import change: the from pyisyox.runtime.node import Node is correctly moved out of TYPE_CHECKING since it's now used at runtime. No circular-import risk (node.py doesn't import from group.py).

Tests cover all four cases and the independence from targets_resolved is explicitly locked in (test_group_has_dimmable_members_independent_of_targets_resolved). The nodedef ids "DimmerLampSwitch" / "RelayLampOnly" — are those present in the test profile fixtures? Worth a quick sanity check if a future test run shows unexpected False from is_dimmable.

---

Minor Observation: Redundant set_route in test_connect_runs_full_load_with_real_profile_fixture

test_client.py:206 explicitly scripts GET /api/groups even though FakeSession.__init__ now defaults it to an empty payload. Harmless redundancy — the _routes dict takes priority over _defaults so both work — and keeping the full-load test self-contained (all routes explicit) is arguably better style for that test. Just noting it isn't a bug.

---

Overall

All previous nits resolved, new has_dimmable_members cleanly designed and tested, _get_json_or_empty docstring accurate, rank comparison now first-seen-wins. Ready to merge.

[shbatm]

Thanks for the thorough review — addressed in 4582da2, plus folded in a related capability.

_link_intent "" sentinel — added a comment at the return "" noting it skips just this link, distinct from None which aborts the whole group.

rank >= → > — changed to strict > (first-seen wins among duplicate same-type links; cross-type precedence native>cmd>default unchanged since 3>1 etc.). Comment explains why.

no-ctl edge — added test_apply_targets_no_ctl_block_resolves_empty ({"id": "G"}, no ctl key → targets_resolved=True, member_intents={}).

_get_json_or_empty docstring — corrected to "swallows any HTTPError → {} (404, 401, 500, …)", noting it mirrors _get_text_or_empty.

reverse-order precedence — added test_apply_targets_native_beats_default_reverse_order (native before default → native still wins).

resolved on-set all stateless — added test_group_all_on_resolved_on_set_all_stateless_is_false documenting the silent-False (pre-existing legacy behaviour, now also covered for the on-set path).

Folded in Group.has_dimmable_members (per maintainer ask, same Group-capability theme): True iff any member node is_dimmable (nodedef-derived; independent of /api/groups so it works on older firmware). With has_state_target it gives the consumer the two orthogonal bits to choose the scene's HA platform — no state target → button; dimmable members → on/off light (preserving Entity.group/more-info, dropping switch_as_x for scenes); else → switch. Note recorded in code + PR: scenes have no settable brightness (fade/brt/dim are separate manual commands, reachable via send_node_command; scene accepts aren't exposed as buttons yet), so the light is on/off only. Added to to_dict; 4 new tests incl. the legacy/unresolved path.

867 passed · mypy clean · pylint 10/10 · ruff clean. PR description updated.

[github-actions]

Claude finished @shbatm's task in 2m 8s —— View job

---

Code Review: PR #175 — feat(group): target-aware scene status from /api/groups link data

Tasks:

• Gather context and understand the PR
• Review latest commit (a3a048e — memoise has_dimmable_members)
• Review pyisyox/runtime/group.py changes
• Review test changes
• Compile and post final review

---

Summary

The latest commit (a3a048e) correctly addresses the non-blocking has_dimmable_members memoization observation from the second review pass. Everything looks good — no new issues.

---

Latest Commit: a3a048e — memoise has_dimmable_members

Memoization is sound and correctly scoped.

The _dimmable_cache: bool | None = None slot is added in __slots__, initialized in __init__, and populated on first access in has_dimmable_members (group.py:218–230). The invariant holds: member_addresses is a tuple (immutable on the record), nodedef_id is static after load, and there's no public setter for _nodes — so the cached value is stable for the lifetime of the Group object. Refreshes create new GroupRecord / Group objects from scratch, so no stale-cache problem across refresh() cycles.

The break on first dimmable match (group.py:228) is a nice short-circuit improvement over the previous early-return True form — semantically identical but cleaner with the new two-phase result + cache pattern.

The comment in __init__ (group.py:82–85) clearly distinguishes why this is safe to cache (static nodedef data) vs. why group_*_on must not be cached (live ST reads via WS dispatcher). That distinction is important and worth having in code, not just in review notes.

Test comment clarifies the intentional redundancy.

test_client.py:206–208 now explains that the explicit set_route("GET", "/api/groups", …) in test_connect_runs_full_load_with_real_profile_fixture coexists with the FakeSession default intentionally — the full-load test pins every route so the fanout call-count assertion is self-contained and doesn't silently depend on FakeSession internals. Good documentation that prevents future "cleanup" from breaking the test's integrity guarantee.

---

All Previous Review Items Addressed ✓

Item	Commit
>= → > for same-type duplicate links (first-seen wins)	4582da2
Reverse insertion order (native before default) test	4582da2
Missing test for absent ctl key in group entry	4582da2
_get_json_or_empty docstring ("any HTTPError", not just 404)	4582da2
Stateless-only on-set causing silent False — documented in test	4582da2
_link_intent "" sentinel comment (skip-link vs. abort-group)	4582da2
has_dimmable_members memoisation (avoids per-call Node construction)	a3a048e
Full-load test /api/groups route comment (self-contained intent)	a3a048e

---

Overall

Design is sound throughout: best-effort /api/groups enrichment with conservative fallback, correct _on_set() / has_state_target / has_dimmable_members separation, memoization correctly scoped to static data, and all review items from both passes resolved. Ready to merge.

[shbatm]

Both non-blocking observations addressed in the latest push.

has_dimmable_members per-call Node construction — now memoised on the instance (_dimmable_cache slot, computed once on first access). Safe to cache precisely because it's nodedef-derived and static for the record — explicitly unlike the live group_*_on aggregates, which must re-read ST and are deliberately left uncached. Docstring notes the memoisation.

Redundant explicit /api/groups route in test_connect_runs_full_load_with_real_profile_fixture — kept (your "all routes explicit is arguably better style for the full-load test" call) and added a comment stating it's intentional despite the FakeSession default, so it isn't later removed as dead.

DimmerLampSwitch / RelayLampOnly are confirmed present in the bundled test profile (same fixture test_node_introspection.py pins is_dimmable against — test_is_dimmable_true_for_dimmer_with_param_don / _false_for_relay_only).

867 passed · mypy clean · pylint 10/10 · ruff clean.