Skip to content

Commit 52c590f

Browse files
kingpanther13claude
andcommitted
fix(flows): keep the caller's intent across a redeclared optional field
begin_step clears only filled, and the caller's key is popped from remaining_config by the first step — so a later step declaring the same optional field reached _redeclared_field_submission with nothing marking it as the caller's, and keep_current_values handed it straight to the backfill. That resubmitted the entry's stored value: a null clear was silently undone, and a caller's new value was overwritten. _ReuseState.recorded_value survives the whole walk (only filled is per-step) and distinguishes a recorded None from no record, so it now decides first: a recorded None keeps the field omitted, a recorded value is resubmitted as the caller's through the existing claim_write path, and only a field the caller never named falls through to the step's own value. The required-field asymmetry predates this PR and is left alone. 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 d7f253d commit 52c590f

2 files changed

Lines changed: 128 additions & 5 deletions

File tree

src/ha_mcp/tools/config_entry_flow_form.py

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -442,11 +442,14 @@ def _redeclared_field_submission(
442442
443443
1. The field is not required, or reuse is barred for this site because the
444444
caller named neither it nor the section holding it (``allow_reuse``).
445-
Under ``keep_current_values`` the step's own value still goes back, per
446-
:func:`_current_value_backfill` — it is the step's data, not the
447-
caller's, so barring reuse does not bar it, and a section the backfill
448-
itself materializes has to carry what it requires. Otherwise omit:
449-
injecting into either on a create flow would invent data.
445+
Under ``keep_current_values`` a value the caller recorded ANYWHERE
446+
earlier in this walk wins first — resubmitted as theirs, or omitted
447+
when it was ``None``, which is the clear. Failing that the step's own
448+
value goes back, per :func:`_current_value_backfill` — it is the step's
449+
data, not the caller's, so barring reuse does not bar it, and a section
450+
the backfill itself materializes has to carry what it requires.
451+
Otherwise omit: injecting into either on a create flow would invent
452+
data.
450453
2. The step's own schema supplies a value — a suggestion or a constant's
451454
only legal value, per :func:`_step_owned_submission_value`: submit that.
452455
It is schema data rather than a caller key, so it is neither marked
@@ -476,6 +479,20 @@ def _redeclared_field_submission(
476479
if not allow_reuse or not field.get("required"):
477480
if not keep_current_values:
478481
return _NO_SUBMISSION
482+
# The caller's intent outranks the step's stored value for the WHOLE
483+
# walk. ``begin_step`` clears only ``filled``, so by a later step the
484+
# caller's key is gone from ``remaining_config`` and nothing here
485+
# marks the field as theirs — but ``scoped``/``flat`` still hold what
486+
# they asked for. Backfilling over that resubmitted the stored value
487+
# and undid it: a recorded ``None`` is the clear this mode documents,
488+
# and a recorded value is the one the caller asked to write.
489+
recorded = reuse_state.recorded_value(path_prefix, name)
490+
if recorded is not _MISSING_DEFAULT:
491+
if recorded is None:
492+
return _NO_SUBMISSION
493+
if not reuse_state.claim_write(dotted):
494+
return _NO_SUBMISSION
495+
return recorded, True
479496
return _current_value_backfill(field)
480497
step_owned = _step_owned_submission_value(field)
481498
if step_owned is not _MISSING_DEFAULT:

tests/src/unit/test_flow_options_preserve.py

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,112 @@ async def test_required_redeclared_field_still_reuses_the_caller_value(
525525
}
526526
assert result["warnings"] == [_reuse_warning("friendly_name", "details")]
527527

528+
@staticmethod
529+
def _redeclaring_step(step_id: str) -> dict[str, Any]:
530+
"""Two steps both declaring the same OPTIONAL field, both pre-filled."""
531+
return {
532+
"type": "form",
533+
"step_id": step_id,
534+
"data_schema": [
535+
{
536+
"name": "province",
537+
"required": False,
538+
"optional": True,
539+
"description": {"suggested_value": "BW"},
540+
"selector": {"select": {"options": ["BW", "BY"]}},
541+
},
542+
],
543+
}
544+
545+
def test_a_clear_survives_a_later_step_redeclaring_the_field(self) -> None:
546+
"""``null`` must stay a clear for the whole walk, not just one step.
547+
548+
``begin_step`` clears only ``filled``, and the caller's key was popped
549+
from ``remaining_config`` by the first step — so by the second step
550+
nothing marks the field as the caller's and the backfill used to
551+
resubmit the stored value, silently undoing the clear the tool
552+
descriptions promise (Patch76 review, #2256).
553+
"""
554+
reuse_state = _ReuseState()
555+
remaining: dict[str, Any] = {"province": None}
556+
557+
first = _handle_form_step(
558+
"flow-2254",
559+
self._redeclaring_step("one"),
560+
remaining,
561+
None,
562+
set(),
563+
reuse_state,
564+
keep_current_values=True,
565+
)
566+
second = _handle_form_step(
567+
"flow-2254",
568+
self._redeclaring_step("two"),
569+
remaining,
570+
None,
571+
set(),
572+
reuse_state,
573+
keep_current_values=True,
574+
)
575+
576+
assert "province" not in first
577+
assert "province" not in second, (
578+
f"The later step resurrected the cleared field: {second}"
579+
)
580+
581+
def test_a_callers_value_outranks_a_later_steps_suggestion(self) -> None:
582+
"""The caller asked for BY; the step still suggests the stored BW."""
583+
reuse_state = _ReuseState()
584+
remaining: dict[str, Any] = {"province": "BY"}
585+
586+
first = _handle_form_step(
587+
"flow-2254",
588+
self._redeclaring_step("one"),
589+
remaining,
590+
None,
591+
set(),
592+
reuse_state,
593+
keep_current_values=True,
594+
)
595+
second = _handle_form_step(
596+
"flow-2254",
597+
self._redeclaring_step("two"),
598+
remaining,
599+
None,
600+
set(),
601+
reuse_state,
602+
keep_current_values=True,
603+
)
604+
605+
assert first["province"] == "BY"
606+
assert second["province"] == "BY", (
607+
f"The later step overwrote the caller's value: {second}"
608+
)
609+
610+
def test_create_flow_still_drops_a_redeclared_optional_field(self) -> None:
611+
"""Flag off keeps the pre-#2254 shape: nothing goes back at all."""
612+
reuse_state = _ReuseState()
613+
remaining: dict[str, Any] = {"province": "BY"}
614+
615+
_handle_form_step(
616+
"flow-2254",
617+
self._redeclaring_step("one"),
618+
remaining,
619+
None,
620+
set(),
621+
reuse_state,
622+
)
623+
second = _handle_form_step(
624+
"flow-2254",
625+
self._redeclaring_step("two"),
626+
remaining,
627+
None,
628+
set(),
629+
reuse_state,
630+
)
631+
632+
assert second == {}
633+
528634
def test_required_field_with_only_a_static_default_is_still_omitted(self) -> None:
529635
"""Voluptuous fills it, exactly as it does for the UI's own form."""
530636
step: dict[str, Any] = {

0 commit comments

Comments
 (0)