Skip to content

Commit 052fab0

Browse files
kingpanther13claude
andcommitted
fix(flows): make a clear hold past its first encounter
What expresses a clear depends on the field's shape, and only one of the two sites deciding it carried the qualifier. _consume_leaf_field submits a null verbatim for a field with a "default" — omitting it there would let voluptuous substitute that default — while _edit_mode_submission omitted any recorded None. So a defaulted field cleared on step one had the static default handed back on its second encounter, reporting success having cleared nothing. Reachable by a later step redeclaring the field or a menu loop revisiting the same one. Both sites now share _clears_by_omission, so they cannot drift again. Also pins the warning the optional reuse path now emits: resubmitting the caller's value is the intended outcome there, so the note rides with it. Found by Patch76 review on #2256. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
1 parent 39e0a95 commit 052fab0

2 files changed

Lines changed: 113 additions & 7 deletions

File tree

src/ha_mcp/tools/config_entry_flow_form.py

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,24 @@ def _current_value_backfill(field: dict[str, Any]) -> tuple[Any, bool]:
423423
return step_owned, False
424424

425425

426+
def _clears_by_omission(field: dict[str, Any]) -> bool:
427+
"""Whether leaving this field OUT is how the caller clears it.
428+
429+
Only for a field that is neither required nor carrying a ``"default"``:
430+
there, an absent key simply stays absent. Anything else has a value
431+
voluptuous substitutes for the omission — the static default, which is a
432+
different value than the caller asked for and would report success while
433+
clearing nothing — so the ``None`` is submitted instead and Home Assistant
434+
decides whether null is meaningful for that field.
435+
436+
The single source of truth for both sites that answer this question:
437+
:func:`_consume_leaf_field`, where the caller's key is popped, and
438+
:func:`_edit_mode_submission`, where a later encounter of the same field
439+
has to reach the same verdict (Patch76 review, issue #2254).
440+
"""
441+
return not field.get("required") and "default" not in field
442+
443+
426444
def _edit_mode_submission(
427445
field: dict[str, Any],
428446
name: str,
@@ -448,7 +466,9 @@ def _edit_mode_submission(
448466
if recorded is _MISSING_DEFAULT:
449467
return _current_value_backfill(field)
450468
if recorded is None:
451-
return _NO_SUBMISSION
469+
# The caller cleared it. Express that the same way the popping site
470+
# did, or the clear stops holding at its second encounter.
471+
return _NO_SUBMISSION if _clears_by_omission(field) else (None, True)
452472
if not reuse_state.claim_write(dotted):
453473
# The one reused write per (step, path) is spent — a menu loop is
454474
# revisiting this step. Falling back to omission would let voluptuous
@@ -569,12 +589,7 @@ def _consume_leaf_field(
569589
"""
570590
if name in remaining_config:
571591
value = remaining_config.pop(name)
572-
clearing = (
573-
keep_current_values
574-
and value is None
575-
and not field.get("required")
576-
and "default" not in field
577-
)
592+
clearing = keep_current_values and value is None and _clears_by_omission(field)
578593
form_data[name] = _CLEARED if clearing else value
579594
_mark_consumed(consumed_config_keys, path_prefix, name)
580595
if reuse_state is not None:

tests/src/unit/test_flow_options_preserve.py

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,97 @@ def test_a_callers_value_outranks_a_later_steps_suggestion(self) -> None:
607607
f"The later step overwrote the caller's value: {second}"
608608
)
609609

610+
@staticmethod
611+
def _defaulted_step(step_id: str) -> dict[str, Any]:
612+
"""One OPTIONAL field carrying a static default and no suggestion."""
613+
return {
614+
"type": "form",
615+
"step_id": step_id,
616+
"data_schema": [
617+
{
618+
"name": "workdays",
619+
"required": False,
620+
"optional": True,
621+
"default": ["mon", "tue", "wed", "thu", "fri"],
622+
},
623+
],
624+
}
625+
626+
@pytest.mark.parametrize(
627+
("first_step", "second_step"),
628+
[("one", "two"), ("init", "init")],
629+
ids=["later-step-redeclares", "same-step-revisited"],
630+
)
631+
def test_a_clear_on_a_defaulted_field_survives_its_second_encounter(
632+
self, first_step: str, second_step: str
633+
) -> None:
634+
"""What expresses a clear depends on the field's shape, at BOTH sites.
635+
636+
``_consume_leaf_field`` submits the ``None`` verbatim for a field
637+
carrying a ``"default"``, because omitting it there would let
638+
voluptuous substitute that default. The later encounter has to reach
639+
the same verdict — omitting it on encounter two hands the default back
640+
and reports success having cleared nothing (Patch76 review, #2256).
641+
"""
642+
reuse_state = _ReuseState()
643+
remaining: dict[str, Any] = {"workdays": None}
644+
645+
first = _handle_form_step(
646+
"flow-2254",
647+
self._defaulted_step(first_step),
648+
remaining,
649+
None,
650+
set(),
651+
reuse_state,
652+
keep_current_values=True,
653+
)
654+
second = _handle_form_step(
655+
"flow-2254",
656+
self._defaulted_step(second_step),
657+
remaining,
658+
None,
659+
set(),
660+
reuse_state,
661+
keep_current_values=True,
662+
)
663+
664+
assert first == {"workdays": None}
665+
assert second == {"workdays": None}, (
666+
"The clear stopped holding at its second encounter; omitting the "
667+
f"key lets the static default back in. Got: {second}"
668+
)
669+
670+
def test_a_redeclared_optional_field_warns_when_it_resubmits(self) -> None:
671+
"""Routing the optional path through claim_write emits its note.
672+
673+
Pinned rather than asserted-away: the mode treats resubmitting the
674+
caller's value as the CORRECT outcome here, so the warning rides along
675+
with it and callers see it (Patch76 review, #2256).
676+
"""
677+
reuse_state = _ReuseState()
678+
remaining: dict[str, Any] = {"province": "BY"}
679+
680+
_handle_form_step(
681+
"flow-2254",
682+
self._redeclaring_step("one"),
683+
remaining,
684+
None,
685+
set(),
686+
reuse_state,
687+
keep_current_values=True,
688+
)
689+
_handle_form_step(
690+
"flow-2254",
691+
self._redeclaring_step("two"),
692+
remaining,
693+
None,
694+
set(),
695+
reuse_state,
696+
keep_current_values=True,
697+
)
698+
699+
assert reuse_state.notes == [_reuse_warning("province", "two")]
700+
610701
def test_a_revisited_step_keeps_the_stored_value_after_the_write_is_spent(
611702
self,
612703
) -> None:

0 commit comments

Comments
 (0)