Cherry-pick upstream #1462 + #1379 (expand_light_groups + race-condition regression test)#12
Conversation
Regression test for basnijholt#1373 This test verifies that when `separate_turn_on_commands: true` is enabled and a light is turned off between split commands, the second command should be skipped. The bug occurs because the proactive adaptation context is never cleared when `light.turn_off` is called, causing the off-check to be bypassed in `_execute_adaptation_calls`. This test currently FAILS (demonstrating the bug) and will PASS after the fix is implemented.
Some light group entities act as a proxy that must receive a single combined `light.turn_on` call to function correctly — for example virtual mixers like <https://github.com/mion00/color-temperature-light-mixer> for instance, that blend a warm and a cold white channel into one entity. In such setups the individual member entities only expose `ColorMode.BRIGHTNESS`, so sending separate per-member commands bypasses the mixing logic. Setting `expand_light_groups: false` keeps the group entity in `self.lights` instead of expanding it to its members. Adaptation commands go to the group, and the interceptor no longer skips group entities for that switch. Default is `true` — no behaviour change for existing configurations.
Conflicts resolved: - const.py: keep both CONF_ENABLE_DIAGNOSTIC_SENSORS (fork) and new CONF_EXPAND_LIGHT_GROUPS validator entries - strings.json / en.json / de.json: keep fork's 5-step layout; add expand_light_groups strings alongside existing entries
…mmands race condition (basnijholt) Conflicts resolved: - tests/test_switch.py: kept fork's test_automation_turn_on_from_off_not_marked_as_manual_control (from basnijholt#1378 fix) and appended upstream's test_separate_turn_on_commands_respects_light_off_state at end of file so both regression tests coexist.
The test documents an unfixed bug (proactive adaptation context not cleared on light.turn_off, causing second split command to be sent to an off light). Upstream basnijholt#1379 added the test without the fix, so we xfail it until the bug is addressed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughIntroduces a new boolean configuration option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/adaptive_lighting/strings.json (1)
26-27:⚠️ Potential issue | 🟠 MajorRemove the raw URL from this description.
The current hassfest failure is for this field:
strings.jsondescriptions cannot contain URLs. Move the link to description placeholders or remove it, otherwise the validation job will keep failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/adaptive_lighting/strings.json` around lines 26 - 27, The "description" value under the "Essentials" entry in strings.json contains a raw URL which violates hassfest; remove the raw link from the description (or replace it with a neutral phrase like "see our web app for interactive graphs" using no URL) and, if you need to preserve the link, move it into a separate description placeholder or resources field instead of the user-facing "description" string so the strings.json entry (key "description" for title "Essentials") no longer contains an inline URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/adaptive_lighting/const.py`:
- Around line 330-335: The option CONF_EXPAND_LIGHT_GROUPS (with
DEFAULT_EXPAND_LIGHT_GROUPS) is defined but not exposed in the options flow; add
CONF_EXPAND_LIGHT_GROUPS to the STEP_OPTIONS["init"] configuration so it appears
in the 5-step options UI (ensure it follows the same pattern as other boolean
options in STEP_OPTIONS["init"] and uses the same label/translation key used in
DOCS). Locate STEP_OPTIONS["init"] in the options flow code and insert a
reference to CONF_EXPAND_LIGHT_GROUPS alongside the other option keys so the UI
renders and persists this setting.
In `@custom_components/adaptive_lighting/switch.py`:
- Around line 955-956: The code mutates self.lights during expansion so toggling
_expand_light_groups_flag and calling _expand_light_groups() loses the original
configured group entities; preserve the original configured light IDs by adding
and using a separate attribute (e.g. self._configured_lights or
self._original_lights) that is set once from the incoming config and never
overwritten by expansions, update _expand_light_groups() to derive expanded
all_lights from that preserved list instead of self.lights, and ensure
change_switch_settings (the path that toggles _expand_light_groups_flag) and any
code that updates manager.lights use the preserved configured list to restore
group entities when expansion is turned off.
- Around line 2003-2008: The intercept was updated to allow group entities when
switch._expand_light_groups_flag is False, but callers still resolve switches
with the default expand_light_groups=True, causing group-targeted calls to miss
the configured switch; update handle_apply, handle_set_manual_control,
set_light_status_for_switches, and reset to pass the switch's
_expand_light_groups_flag (or False when referenced) into calls to
_switch_with_lights() and _switches_with_lights() so they resolve group entities
consistently with the switch configuration, ensuring group-targeted service
calls find the correct switch and avoid NoSwitchFoundError or skipped updates.
In `@custom_components/adaptive_lighting/translations/de.json`:
- Around line 37-38: Add the missing German helper text for the translation key
options.step.init.data_description.expand_light_groups by adding the appropriate
German string to the translations JSON (the key currently missing in
translations/de.json); locate the expand_light_groups entry used alongside
room_preset in the same JSON and insert the German equivalent of the English
helper (explaining that expand_light_groups controls whether group entities are
expanded to individual member entities (`true`) or the group entity is
controlled directly (`false`)), ensuring proper JSON quoting and comma placement
to match surrounding entries.
In `@tests/test_switch.py`:
- Around line 3039-3049: The variable name turn_off_time is misleading because
it's used as a boolean flag; rename it to after_turn_off everywhere it's
declared and referenced (including its initial assignment and within the async
function track_turn_on_calls where it sets "after_turn_off": turn_off_time is
not None), and update any other uses (e.g., places that set it to True/False or
check it) so that the flag semantics match the new name and the test list
turn_on_calls continues to record the "after_turn_off" value correctly.
---
Outside diff comments:
In `@custom_components/adaptive_lighting/strings.json`:
- Around line 26-27: The "description" value under the "Essentials" entry in
strings.json contains a raw URL which violates hassfest; remove the raw link
from the description (or replace it with a neutral phrase like "see our web app
for interactive graphs" using no URL) and, if you need to preserve the link,
move it into a separate description placeholder or resources field instead of
the user-facing "description" string so the strings.json entry (key
"description" for title "Essentials") no longer contains an inline URL.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f33d395b-0f67-4739-aa8f-80046c912046
📒 Files selected for processing (6)
custom_components/adaptive_lighting/const.pycustom_components/adaptive_lighting/strings.jsoncustom_components/adaptive_lighting/switch.pycustom_components/adaptive_lighting/translations/de.jsoncustom_components/adaptive_lighting/translations/en.jsontests/test_switch.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Run pytest (2026.2.2, 3.13.2)
- GitHub Check: Run pytest (dev, 3.14.2)
- GitHub Check: Run pytest (2025.8.3, 3.13.2)
- GitHub Check: Run pytest (2025.9.4, 3.13.2)
- GitHub Check: Run pytest (2025.6.3, 3.13.2)
- GitHub Check: Run pytest (2025.11.3, 3.13.2)
- GitHub Check: Run pytest (2025.12.5, 3.13.2)
- GitHub Check: Run pytest (2025.7.4, 3.13.2)
- GitHub Check: Run pytest (2025.3.4, 3.13.0)
- GitHub Check: Run pytest (2024.12.5, 3.12.0)
- GitHub Check: Run pytest (2026.1.3, 3.13.2)
- GitHub Check: Run pytest (2025.5.3, 3.13.2)
- GitHub Check: Run pytest (2025.10.4, 3.13.2)
- GitHub Check: Run pytest (2025.4.4, 3.13.0)
- GitHub Check: Run pytest (2025.2.5, 3.13.0)
- GitHub Check: Run pytest (2025.1.4, 3.12.0)
- GitHub Check: build
- GitHub Check: pre-commit
- GitHub Check: markdown-code-runner
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
Focus on: error handling, type safety, async correctness, and missing edge cases. Flag any hardcoded credentials or API keys.
Files:
custom_components/adaptive_lighting/const.pycustom_components/adaptive_lighting/switch.pytests/test_switch.py
**/tests/**
⚙️ CodeRabbit configuration file
Prioritize missing edge case coverage and regression risks over style nits.
Files:
tests/test_switch.py
🪛 GitHub Actions: Validate with hassfest
custom_components/adaptive_lighting/strings.json
[error] 1-1: hassfest TRANSLATIONS: Invalid strings.json for integration 'adaptive_lighting' (/github/workspace/custom_components/adaptive_lighting/). The string should not contain URLs. Replace the URL with description placeholders for dictionary value @ data['options']['step']['init']['description']. Got 'Pick your lights and set brightness and color ranges. Most users only need this page. For interactive graphs, visit this web app.'
🔇 Additional comments (3)
tests/test_switch.py (3)
2998-3018: LGTM! Well-documented regression test.The
xfail(strict=False)marker is appropriate for documenting an unresolved upstream bug. The docstring clearly explains the race condition: proactive adaptation context isn't cleared onlight.turn_off, causing the second split command to bypass the off-check in switch.py:1378-1400 (as shown in the relevant code snippets).
3066-3104: LGTM! Correctly captures the race condition timing.The test flow properly exercises the bug scenario:
- Turn on with tracked context (starts proactive adaptation with split commands)
- Verify proactive context is set
- Immediately turn off (before second split command completes)
- Wait for all adaptation tasks via
asyncio.gather- Check if proactive context persists (root cause indicator)
The use of
blocking=Truefor service calls combined withasyncio.gatherfor adaptation tasks correctly captures the race condition window.
3107-3144: LGTM! Comprehensive bug verification with clear failure messages.The assertions check all manifestations of the documented bug:
- Proactive context remaining set after
turn_off(root cause)- Light state being ON after
turn_offwas called- Any
light.turn_oncalls occurring afterturn_offThe entity ID check correctly handles both single-entity and multi-entity service call formats. The detailed
pytest.fail()messages clearly explain the expected fix per the relevant code snippets (clearing proactive context whenlight.turn_offis detected).
| self._expand_light_groups_flag = data[CONF_EXPAND_LIGHT_GROUPS] | ||
| self._expand_light_groups() # updates manual control timers |
There was a problem hiding this comment.
Preserve the original configured light list when toggling this flag.
_expand_light_groups() now derives the next all_lights from self.lights, but self.lights has already been mutated by prior expansions. After one expand_light_groups=True pass, switching it back to false via change_switch_settings can no longer restore the group entity, and the old member IDs stay registered in manager.lights.
As per coding guidelines, focus on missing edge cases.
Also applies to: 1040-1043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@custom_components/adaptive_lighting/switch.py` around lines 955 - 956, The
code mutates self.lights during expansion so toggling _expand_light_groups_flag
and calling _expand_light_groups() loses the original configured group entities;
preserve the original configured light IDs by adding and using a separate
attribute (e.g. self._configured_lights or self._original_lights) that is set
once from the incoming config and never overwritten by expansions, update
_expand_light_groups() to derive expanded all_lights from that preserved list
instead of self.lights, and ensure change_switch_settings (the path that toggles
_expand_light_groups_flag) and any code that updates manager.lights use the
preserved configured list to restore group entities when expansion is turned
off.
| # Never adapt on light groups when expanding, because HA will make a separate light.turn_on | ||
| or ( | ||
| switch._expand_light_groups_flag | ||
| and (e := self.hass.states.get(entity_id)) | ||
| and _is_light_group(e) | ||
| ) |
There was a problem hiding this comment.
Group-preserving mode still breaks group-targeted service resolution.
Allowing group entities through this intercept path is only half of the change. handle_apply, handle_set_manual_control, set_light_status_for_switches, and reset still resolve switches through _switch_with_lights() / _switches_with_lights() with the default expand_light_groups=True. For a switch configured with expand_light_groups=False, a call that targets the group entity no longer matches its switch and can raise NoSwitchFoundError or skip reset/status updates.
As per coding guidelines, focus on missing edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@custom_components/adaptive_lighting/switch.py` around lines 2003 - 2008, The
intercept was updated to allow group entities when
switch._expand_light_groups_flag is False, but callers still resolve switches
with the default expand_light_groups=True, causing group-targeted calls to miss
the configured switch; update handle_apply, handle_set_manual_control,
set_light_status_for_switches, and reset to pass the switch's
_expand_light_groups_flag (or False when referenced) into calls to
_switch_with_lights() and _switches_with_lights() so they resolve group entities
consistently with the switch configuration, ensuring group-targeted service
calls find the correct switch and avoid NoSwitchFoundError or skipped updates.
| "room_preset": "Room preset", | ||
| "expand_light_groups": "expand_light_groups: Lichtgruppen auf einzelne Mitgliedsentitäten erweitern (`true`) oder die Gruppenentität direkt steuern (`false`)." |
There was a problem hiding this comment.
Add the German helper text for expand_light_groups.
strings.json and translations/en.json both define options.step.init.data_description.expand_light_groups, but translations/de.json still omits it. German users will get a missing or fallback-English helper text for the new option.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@custom_components/adaptive_lighting/translations/de.json` around lines 37 -
38, Add the missing German helper text for the translation key
options.step.init.data_description.expand_light_groups by adding the appropriate
German string to the translations JSON (the key currently missing in
translations/de.json); locate the expand_light_groups entry used alongside
room_preset in the same JSON and insert the German equivalent of the English
helper (explaining that expand_light_groups controls whether group entities are
expanded to individual member entities (`true`) or the group entity is
controlled directly (`false`)), ensuring proper JSON quoting and comma placement
to match surrounding entries.
| turn_off_time = None | ||
|
|
||
| async def track_turn_on_calls(event: Event) -> None: | ||
| if ( | ||
| event.data.get("domain") == LIGHT_DOMAIN | ||
| and event.data.get("service") == SERVICE_TURN_ON | ||
| ): | ||
| turn_on_calls.append( | ||
| {"event": event, "after_turn_off": turn_off_time is not None}, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: turn_off_time is misleading variable name.
The variable is used as a boolean flag (True) but is named as if it stores a timestamp. Consider renaming to after_turn_off for clarity.
💡 Suggested rename for clarity
- turn_off_time = None
+ turn_off_happened = False
async def track_turn_on_calls(event: Event) -> None:
if (
event.data.get("domain") == LIGHT_DOMAIN
and event.data.get("service") == SERVICE_TURN_ON
):
turn_on_calls.append(
- {"event": event, "after_turn_off": turn_off_time is not None},
+ {"event": event, "after_turn_off": turn_off_happened},
)And at line 3086:
- turn_off_time = True
+ turn_off_happened = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_switch.py` around lines 3039 - 3049, The variable name
turn_off_time is misleading because it's used as a boolean flag; rename it to
after_turn_off everywhere it's declared and referenced (including its initial
assignment and within the async function track_turn_on_calls where it sets
"after_turn_off": turn_off_time is not None), and update any other uses (e.g.,
places that set it to True/False or check it) so that the flag semantics match
the new name and the test list turn_on_calls continues to record the
"after_turn_off" value correctly.
|
Hi Florian, you’re right. I’m kind of distracted by other projects. It’s also that I get a little overwhelmed when looking at all the PRs. Some of them change 4,000 lines, whereas the entire codebase is approximately that size. Even though I use AI myself, I’m a little skeptical about hitting merge without looking at it in detail. After all, 50,000 to 100,000 people use the code. |
Without this, the option is defined in VALIDATION_TUPLES but invisible in the 5-step options flow UI — users can't toggle it and it stays at the default True forever. Caught by /plan-eng-review on PR #12.
…ning (#13) * test: full coverage for expand_light_groups option Adds 5 unit tests filling the P2 gap from /plan-eng-review on PR #12. The new option's non-default branch had zero coverage; these lock in behavior before users start flipping it (e.g. for Lightener, Hue rooms, or Zigbee2MQTT groups). - test_expand_light_groups_const_wiring: default True, VALIDATION_TUPLES, STEP_OPTIONS[workarounds] UI wiring, DOCS entry — catches the exact class of bug that required the follow-up STEP_OPTIONS fix in #12. - test_expand_light_groups_false_keeps_group_entity: group stays in manager.lights, members are NOT tracked individually. - test_expand_light_groups_true_expands_to_members: default behavior still produces the member-level tracking, preventing accidental regression. - test_expand_light_groups_false_non_group_unaffected: no-op for non-group lights — safe for users who flip the flag without groups. - test_expand_light_groups_false_group_turn_on_reaches_group: the core behavioral guarantee — a user turn_on on a group entity must reach the group, not get silently rerouted to members. This is what unblocks Lightener. Full suite: 35 pre-existing failures (unchanged baseline), 0 new regressions, 1 intentional xfail from PR basnijholt#1379. * feat: warn on silent group expansion + adapt-cycle/manual-control tests Given the Lightener curve card work depends on expand_light_groups=False being honored everywhere, this commit closes three silent-failure gaps from the follow-up review on PR #13: Gap 1 — adapt-cycle dispatch test test_expand_light_groups_false_adapt_cycle_dispatches_to_group Spies on _adapt_light to lock in that the periodic adaptation cycle dispatches to the GROUP entity_id, not its members, when flag=False. Without this, the intercept path could behave correctly while the time-interval-driven adaptation silently bypasses Lightener curves on every adapt cycle. Gap 2 — manual-control attribution test test_expand_light_groups_false_no_false_manual_control With take_over_control=True + flag=False, internal member state changes (Lightener fan-out, Hue bridge sync) must NOT populate manager.manual_control. Otherwise every adapt cycle would mark members as manually controlled and the next cycle would skip them entirely — a slow death spiral with curves that eventually never fire. Gap 3 — one-time warning when AL expands a light group switch.py: AdaptiveSwitch._expand_light_groups now emits a WARNING (debounced via manager.expand_light_groups_warned set) pointing the user at the expand_light_groups: false flag. Fires only from the switch-configured code path; utility expansions in _switches_with_lights stay silent so matching doesn't spam the log. test_expand_light_groups_true_emits_warning_once asserts the warning fires exactly once per entity per HA run. test_expand_light_groups_false_does_not_warn asserts the warning stays silent for users who already opted in to the fix. Full suite: 35 pre-existing failures (unchanged baseline), 0 new regressions. 9/9 new expand_light_groups tests passing. 1 intentional xfail from basnijholt#1379 untouched. --------- Co-authored-by: Florian Horner <florianhorner@macbook-pro-von-florian.tail6f28f8.ts.net>
3 participants
Summary
Batch cherry-pick from upstream
basnijholt/adaptive-lightingwhile Bas is focused on other projects.Merged
feat: add expand_light_groups option basnijholt/adaptive-lighting#1462 —
expand_light_groupsoption by @hesseleoconst.py,strings.json,translations/en.json,translations/de.json; fork's 5-step layout preserved and new strings added alongside existing entries.Add regression test for separate_turn_on_commands race condition basnijholt/adaptive-lighting#1379 — Regression test for
separate_turn_on_commandsrace condition by @basnijholttest_separate_turn_on_commands_respects_light_off_statedocumenting a bug where proactive adaptation context isn't cleared onlight.turn_off, causing the second split command to be sent to an off light.@pytest.mark.xfailwith a reason pointing at Add regression test for separate_turn_on_commands race condition basnijholt/adaptive-lighting#1379. The xfail preserves the test as living documentation of the bug without breaking CI.Skipped
Two PRs from the original batch proposal were found to be incompatible with the current fork state:
has_entity_namefix, thelast_service_datamerge from upstream 6cebe14, and the status tracking system. The group concern is largely covered by already-merged fix: prevent transient Zigbee off-state from cancelling adaptation basnijholt/adaptive-lighting#1460.async_setup) — removesPLATFORMS = ["switch", "sensor"](breaks fork's diagnostic sensors), refactors service registration in an incompatible way.Test plan
core@2026.2.2: 35 pre-existing failures (unchanged fromfork/mainbaseline), 0 new regressions, 1 intentionalxfailfor the documented upstream bugGenerated with Claude Code.