Skip to content

Commit 4ab5979

Browse files
kingpanther13claude
andcommitted
fix(flows): reject a malformed step_values directive
step_values is consumed as a directive, so a malformed one was invisible to ignored-key reporting: an outer value that is not an object, or a list entry that is not one, applied nothing and still returned success. Two of the four malformed shapes warned; the other two were silent. validate_step_values runs at the top of both walkers, before any step is driven, and raises ToolError per the repo's tool-failure rule rather than warning — a malformed directive is caller error, not a degraded result. Valid shapes are unchanged: a dict entry, or a list of dicts for a step the flow presents more than once. Found by CodeRabbit review on #2256. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
1 parent 5cfe5f0 commit 4ab5979

3 files changed

Lines changed: 103 additions & 0 deletions

File tree

src/ha_mcp/tools/config_entry_flow_form.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -841,6 +841,52 @@ def _consume_all_remaining_keys(
841841
return form_data
842842

843843

844+
def validate_step_values(config: dict[str, Any]) -> None:
845+
"""Reject a malformed ``step_values`` directive before the walk starts.
846+
847+
The reserved key is consumed as a directive, so a malformed one is invisible
848+
to ignored-key reporting: the walk would complete, report success, and have
849+
applied nothing the caller asked for. A caller error is a tool-level
850+
failure, so it raises rather than warns (CodeRabbit review, issue #2254).
851+
852+
Accepted: a dict of ``step_id -> entry``, where an entry is a dict of field
853+
values, or a LIST of such dicts consumed one per encounter of that step.
854+
"""
855+
directive = config.get(_PER_STEP_VALUES_KEY)
856+
if directive is None:
857+
return
858+
859+
example = "{'<step_id>': {'<field>': <value>}}"
860+
if not isinstance(directive, dict):
861+
raise_tool_error(
862+
create_error_response(
863+
ErrorCode.VALIDATION_INVALID_PARAMETER,
864+
f"'{_PER_STEP_VALUES_KEY}' must be an object keyed by step id, "
865+
f"got {type(directive).__name__}",
866+
suggestions=[f"Pass {_PER_STEP_VALUES_KEY}={example}."],
867+
context={_PER_STEP_VALUES_KEY: directive},
868+
)
869+
)
870+
871+
for step_id, entry in directive.items():
872+
entries = entry if isinstance(entry, list) else [entry]
873+
bad = [item for item in entries if not isinstance(item, dict)]
874+
if bad:
875+
raise_tool_error(
876+
create_error_response(
877+
ErrorCode.VALIDATION_INVALID_PARAMETER,
878+
f"'{_PER_STEP_VALUES_KEY}[{step_id!r}]' must be an object "
879+
"of field values, or a list of them for a step the flow "
880+
f"presents more than once — got {type(bad[0]).__name__}",
881+
suggestions=[
882+
f"Pass {_PER_STEP_VALUES_KEY}={example}, or a list of "
883+
"those objects to supply one per encounter.",
884+
],
885+
context={"step_id": step_id, "entry": entry},
886+
)
887+
)
888+
889+
844890
@contextmanager
845891
def _step_values_applied(
846892
remaining_config: dict[str, Any],

src/ha_mcp/tools/config_entry_flow_walker.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
_handle_form_step,
2525
_ReuseState,
2626
_success_warnings,
27+
validate_step_values,
2728
)
2829
from .config_entry_flow_menu import (
2930
_flow_step_budget,
@@ -854,6 +855,7 @@ async def _handle_flow_steps(
854855
"""
855856
if submit_fn is None:
856857
submit_fn = client.submit_config_flow_step
858+
validate_step_values(config)
857859
remaining_config = dict(config)
858860
current_step = initial_step
859861
last_menu_choice: str | None = None
@@ -1102,6 +1104,7 @@ async def _handle_config_subentry_flow_steps(
11021104
resubmits the step's own value for every declared field the caller named
11031105
no key for, so a partial patch stops wiping the rest of the subentry.
11041106
"""
1107+
validate_step_values(config)
11051108
remaining_config = dict(config)
11061109
current_step = initial_step
11071110
last_menu_choice: str | None = None

tests/src/unit/test_flow_options_preserve.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
_handle_form_step,
3131
_ignored_keys_warnings,
3232
_ReuseState,
33+
validate_step_values,
3334
)
3435
from ha_mcp.tools.config_entry_flow_walker import (
3536
ReconfigureStatus,
@@ -1229,6 +1230,59 @@ def test_a_schemaless_step_does_not_submit_the_reserved_key(self) -> None:
12291230
assert payload == {"host": "10.0.0.5"}
12301231

12311232

1233+
class TestStepValuesValidation:
1234+
"""A malformed directive raises rather than silently applying nothing."""
1235+
1236+
@pytest.mark.parametrize(
1237+
"directive",
1238+
["oops", 42, ["init"]],
1239+
ids=["string", "number", "list"],
1240+
)
1241+
def test_a_non_object_directive_is_rejected(self, directive: Any) -> None:
1242+
with pytest.raises(ToolError) as exc_info:
1243+
validate_step_values({"step_values": directive})
1244+
assert "must be an object keyed by step id" in str(exc_info.value)
1245+
1246+
@pytest.mark.parametrize(
1247+
"entry",
1248+
["TX", 42, ["TX"], [{"a": 1}, "TX"]],
1249+
ids=["string", "number", "list-of-string", "list-with-bad-tail"],
1250+
)
1251+
def test_a_non_object_entry_is_rejected(self, entry: Any) -> None:
1252+
with pytest.raises(ToolError) as exc_info:
1253+
validate_step_values({"step_values": {"init": entry}})
1254+
assert "must be an object of field values" in str(exc_info.value)
1255+
1256+
@pytest.mark.parametrize(
1257+
"config",
1258+
[
1259+
{"province": "BY"},
1260+
{"step_values": {"init": {"province": "TX"}}},
1261+
{"step_values": {"init": [{"province": "TX"}, {"province": "NY"}]}},
1262+
{"step_values": {}},
1263+
],
1264+
ids=["absent", "dict-entry", "list-entry", "empty-directive"],
1265+
)
1266+
def test_valid_shapes_pass(self, config: dict[str, Any]) -> None:
1267+
validate_step_values(config)
1268+
1269+
async def test_the_walker_rejects_before_driving_any_step(self) -> None:
1270+
"""It must raise before a single step is submitted."""
1271+
submit_fn = AsyncMock()
1272+
1273+
with pytest.raises(ToolError):
1274+
await _handle_flow_steps(
1275+
client=None,
1276+
flow_id="flow-2254",
1277+
initial_step=_workday_options_step(),
1278+
config={"days_offset": 3, "step_values": {"init": "TX"}},
1279+
submit_fn=submit_fn,
1280+
keep_current_values=True,
1281+
)
1282+
1283+
submit_fn.assert_not_awaited()
1284+
1285+
12321286
class TestEmptyFormsGuidance:
12331287
"""The empty-forms error must point at the right mistake."""
12341288

0 commit comments

Comments
 (0)