Commit 43a192c
fix: stop partial options-flow submits from wiping unnamed fields (#2256)
* fix(flows): keep unnamed fields when editing an existing config entry
An options, reconfigure or subentry-reconfigure step arrives pre-filled by
Home Assistant's add_suggested_values_to_schema, and saving the UI form posts
every box back. The flow walker submitted only the keys the caller named, so
voluptuous filled each omitted vol.Optional(k, default=STATIC) with its static
default and dropped every no-default optional outright: a one-field patch
through ha_set_integration(entry_id=..., config=...) silently reset the rest of
the entry. Repro on core workday, where config={"days_offset": 3} reset the
workday/exclude lists and wiped province, returning success with no warnings.
Thread keep_current_values through the two flow walkers and the form-step
consumption. Under it, a declared field the caller named no key for is
submitted with the value the step itself carries (suggestion, else a
constant's only legal value), including leaves inside sections the caller
never named, since the section is a box on the same form. A bare "default"
still means omission, exactly as it does for the UI's own form. An explicit
null is the opposite request and is honoured as a clear: consumed, then left
out of the payload. Backfilled values are the step's data, so they neither
count towards the "consumed at least one caller key" guard nor satisfy
reconfigure's "consumed EVERY key" one, and a value carrying a redaction
sentinel is never written back.
The flag is set by update_config_entry_options (ha_set_integration options
mode and helper updates), the official reconfigure flow, and the subentry
reconfigure branch. Create flows - add integration, create helper, create
subentry - are unchanged: there is no stored value to preserve and
materializing a field would invent data.
Fixes #2254
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* refactor(dev): harvest update_source's preserved overrides from the flow schema
Replaces the hardcoded _PRESERVED_OPTION_KEYS tuple with a schema-driven
harvest (description.suggested_value, the same suggestion-over-default
rule as the walker's keep_current_values backfill), so a field added to
the component's options flow later cannot be silently wiped by a partial
update_source submit.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): submit an explicit null for defaulted fields instead of omitting it
Omitting a null'd field that carries a voluptuous default let HA
substitute the static schema default, so the tool reported success
while writing a value the caller never asked for and never cleared the
field. Only a field with no default can express a clear by omission;
everything else submits the null for HA to validate.
Found by Codex review on #2256.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(e2e): stop the HACS nudge lanes racing their own retry schedule
Both positive lanes waited exactly RETRY_DELAYS[0] (30s) for the marker.
The nudge's first attempt is immediate, but a launcher can beat HA to
registering HACS's WS handlers; that attempt returns unknown_command,
which _refresh_with_retries cannot tell apart from 'no HACS' and so
retries at RETRY_DELAYS[0] — landing attempt two AT the deadline. Seen
on the ubuntu-24.04-arm leg of #2256: unknown_command at 02:26:42,
retry at 02:27:13, assert at 30s.
Derives the wait from the schedule so the two cannot drift apart, and
corrects both failure messages, which blamed the lifespan for not
scheduling the nudge when it had scheduled it and was mid-retry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(e2e): budget the nudge wait for a hanging first attempt
The wait covered RETRY_DELAYS[0] but not the time attempt one can spend
before it fails: hacs/repositories/list has no explicit timeout, so it
waits DEFAULT_COMMAND_WAIT_TIMEOUT before giving up, putting the start
of attempt two at the old deadline. Budget both command timeouts and
the retry delay, all derived so neither schedule can outgrow the wait.
Names send_command's default reply timeout, which callers scheduling
retries have to budget around.
Found by CodeRabbit review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(e2e): pin the #2254 partial-options wipe against a real options flow
The existing options round-trip could not catch it: group's LIGHT options
are all vol.Required, and required fields were already backfilled from the
step's suggestion. group_type=sensor is the reachable repro -- its schema
adds vol.Optional(ignore_non_numeric, default=False), the optional +
static-default shape that voluptuous silently refills when the key is
omitted.
Complements the unit suite rather than repeating it: those pin the payload
against a hand-written copy of HA's serialization and would keep passing if
HA changed how it emits suggested_value. Verified to fail pre-fix (field
omitted, persisted False) and pass post-fix (True submitted and kept).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(e2e): use call_tool_success for the #2254 expected-success calls
tests/AGENTS.md line 51 is the convention for success-expecting calls;
the new test had copied the sibling's older call_tool + assert_mcp_success
pattern. call_tool_success also turns a raised ToolError into a named
assertion failure instead of an opaque exception.
Found by CodeRabbit review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(e2e): set the #2254 baseline through the options flow
ignore_non_numeric is in group's SENSOR_OPTIONS, not its config schema, so
setting it on the add call was dropped as an undeclared key and the entry
never had it — the baseline wait timed out on every full-suite leg. Set it
with a full options submit instead, which also serves as the control for
the partial patch that follows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): let an explicit clear beat a required section's prefill
_required_section_defaults seeds a REQUIRED section with the step's own
values before anything is consumed, and _field_default_value reads
description.suggested_value first -- so an optional no-default leaf inside
one arrives already holding its stored value. Omitting the cleared key let
that seed survive the merge: the old value was submitted while the tool
reported a successful clear.
A clear now rides through the merge as a _CLEARED sentinel in the key's
place and is stripped once the section is assembled, so it wins at any
nesting depth and a section holding only clears is dropped rather than
submitted.
Also stops update_source echoing preserved credentials: the component's
options form exposes oauth_client_secret as a suggested_value, so the
schema-driven resend picks it up (correctly -- omitting it would clear it),
but this path submits the flow directly and never meets the flow-schema
redaction. The response now reports only what the caller asked to change.
And budgets the HACS nudge wait for send_hacs_repository_refresh's own
HACS_REFRESH_TIMEOUT, which runs after the retried list call and writes the
marker only once it returns.
Found by Codex review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(dev): pin update_source's response to the caller's requested change
test_non_embedded_never_routes_to_component_write asserted the response
echoed the preserved server_url alongside the channel delta — the contract
that leaked oauth_client_secret. Its subject is the sync-vs-scheduled
routing, so it now asserts the trimmed response AND that the submission
still carries the override, pinning both halves of the fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* 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
* refactor(flows): extract the edit-mode submission decision
The caller-intent-before-backfill branches pushed
_redeclared_field_submission to C901 12 > 10. Repo policy is extract, never
a per-file ignore, and the extracted decision stands on its own: what to
submit for an edit-mode field the caller named no key for at THIS step.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): keep the stored value when a revisited step's write is spent
claim_write allows one reused write per (step, path). Once spent, an
optional edit-mode field fell back to omission — which hands voluptuous
the field's STATIC default and overwrites the entry's stored value, the
exact wipe this mode exists to stop. A menu loop revisiting the same step
is enough to reach it.
The step's own value now goes back instead. The required-field branch
still omits: there, omission raises HA's own loud 'required key not
provided' rather than losing data silently.
Found by CodeRabbit review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* 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
* feat(flows): address a field per step encounter via step_values
The flat config dict is keyed by field NAME alone, so a flow whose steps
declare the same field twice — a later step redeclaring it, or one
revisited through a menu loop — could only ever carry one value for both.
The walker reused that value everywhere and warned that per-visit values
could not be expressed, which is what made the warning a dead end: it
reported a limitation with no remedy, on an outcome that was otherwise
correct.
step_values={'<step_id>': {'<field>': <value>}} supplies the remedy,
mirroring the reserved-key convention next_step_id already uses. A step's
entry shadows the flat value for that step only and is restored after, so
addressing one step never spends the caller's flat value and a step nobody
addresses behaves exactly as before. It buys three things the flat dict
cannot express: a different value per visit, a value on only some visits,
and a clear on one visit but not another.
The warning now names the remedy instead of declaring the case
impossible, which resolves the review point that it fired on a correct
outcome with nothing the caller could do about it.
Found by Patch76 review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): report step_values entries the flow never presented
A step_values key naming a step the walk never reaches — a typo'd step_id,
or a branch the menu selections never took — applied nothing and said so
nowhere. Each step's entry is now spent as it is applied, so whatever
remains at the end is named in warnings, mirroring how an un-consumed menu
selection is reported.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): isolate step_values from reuse and stop it swallowing typos
Two defects in the per-step overlay, plus the CodeQL gate:
1. The overlay goes through the normal leaf consumer, which records what it
consumed in _ReuseState — and flat/scoped survive the whole walk. A value
the caller scoped to one step was therefore reused by a LATER unaddressed
step instead of that step's own stored value. _ReuseState now knows which
names are step-scoped and keeps them out of flat/scoped, while still
marking them filled so nothing is injected over them in their own step.
2. Overlay keys were dropped after the step, so a field the step's schema
never declared vanished before ignored-key accounting saw it — and the
reserved outer key suppressed any warning, letting the update report
success having applied nothing. An unconsumed overlay key is now reported
under its own path (step_values.<step_id>.<field>) before being dropped.
3. _PER_STEP_VALUES_KEY moved to config_entry_flow_form, its only user.
CodeQL's py/unused-global-variable reads a constant defined in one module
and used only from another as dead — the same reason _MENU_SELECTION_KEYS
already lives there. A code fix, not an allowlist entry.
Found by CodeRabbit review and the CodeQL gate on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): point the empty-forms error at step_values step ids
step_values is a directive, not a field, so a config of nothing but a
step_values entry naming a step the flow never presents tripped the
empty-forms guard and was told to check its field names. The likely
mistake there is the step_id. Guidance now branches on what was actually
supplied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): scope nested step_values leaves out of reuse too
step_scoped holds the overlay's TOP-LEVEL keys, but the guard compared the
leaf name — so an overlay key naming a section let every value inside it
through. For {'connection': {'province': 'TX'}} the leaf is 'province',
which is nowhere in {'connection'}, and a later step declaring
connection.province resubmitted TX instead of its stored value.
Matched on the root of the declaration path instead.
Found by CodeRabbit review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): close the flat-key reuse leak and let step_values loop
Two shapes the per-step overlay still got wrong:
1. record() matched only the declaration ROOT against step_scoped, which
catches an overlay key naming a section. The mirror case is a FLAT
overlay key filling a leaf a section declares — the walker accepts that —
where the root is the section name and equally absent. The step-scoped
value then survived the whole walk and was submitted for a step nobody
addressed. Both the popped key and the root are matched now; either check
alone leaks the other shape.
2. A step's entry was spent on its first encounter, so a menu loop got the
scoped value once and the step's own suggestion afterwards — fewer
encounters than the flat key it replaces, which is what claim_write's note
points callers at. A LIST is now consumed one entry per encounter with its
tail replacing the key until dry, mirroring next_step_id, which is the
only shape that can express a loop or a different value per visit. A dict
still applies once.
The leftover warning covers both causes: a step never presented, and a tail
left because the step ran fewer times than the list expects.
Found by Patch76 review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* 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
* fix(flows): reject an explicit null step_values directive
The guard returned early on a falsy directive, so {'step_values': None}
passed validation — and because the reserved key is excluded from
ignored-key reporting, the walk then applied the caller's other fields and
returned a clean success for a directive that did nothing. Key PRESENCE is
the test, so an explicit None now falls through to the not-an-object
rejection.
Found by CodeRabbit review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): keep submitted values out of step_values rejections
Three problems with the boundary rejection:
1. The error context echoed the caller's entry verbatim, and a rejected
directive can carry a credential the caller is submitting for the FIRST
time. Home Assistant does not hold it yet, so no read-back has harvested
it and RedactSecretsMiddleware cannot scrub it even when enabled — and
raise_tool_error serialises the whole response into the exception
message, which @log_tool_usage records as error_message from inside the
tool, where only 'parameters' are masked. The value reached plaintext
mcp_usage.jsonl. Reports received_type / entry_types now; the shape is
what diagnoses a malformed directive anyway.
2. Entries that apply nothing were reported inconsistently: a bare {} left
a leftover the warning named, while [] and [{}] were popped at their
first encounter and vanished, so the walk reported a clean success for a
directive that did nothing. All four shapes reject now.
3. The list form was taught only by the rejection text. It is on all three
composing surfaces now, including the resubmission note — the one that
fires in the revisited-step case a single dict cannot express.
Found by Patch76 review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): report an entry position, not the caller's step id
A step id is a caller-controlled key, and these rejections reach the usage
log unmasked the same way a value would. Nothing echoed a NESTED caller key
before this directive existed — the walker's supplied_keys reports only
top-level names — so this was new exposure rather than an existing one.
Errors now name the entry's position (step_values entry #2) and its types.
That still says which entry is wrong, and cannot carry anything the caller
typed.
Found by CodeRabbit review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* fix(flows): scope the no-echo rule to values, keep the per-encounter no-op
Three points from review, all measured first:
- The comment justifying position-over-step-id claimed nothing echoed a
nested caller key. Field names do: step_values.<step>.<field> goes into
ignored_config_keys, which _unconsumed_reconfigure_keys unions into the
reconfigure abort error -- the same logged path. That is kept, because
naming the field is the whole diagnostic for a typo and a name is not the
secret a value is, so the comment is scoped to values instead.
- An empty object INSIDE a longer list is a capability, not a mistake: it
means "leave this encounter as it would be without the directive", which
nothing else expresses -- omitting the step affects every encounter, and
{"field": None} is an explicit clear. Documented in the validator
docstring and pinned by a test; entries that apply nothing anywhere are
still rejected.
- context={} was a no-op (create_error_response gates on truthiness) and
the guard reduced to "not any(entries)". Both simplified.
Verified live on a real workday options flow before fixing: the [{}, ...]
shape is accepted, the empty entry is a genuine no-op, and the tail is
reported.
Found by Patch76 review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
* test(flows): actually pin the per-encounter no-op
The test demonstrated the fallback and guarded the dead shapes in two
halves that never met: the demonstrating half drives _handle_form_step,
which never calls validate_step_values -- only the walkers do. So nothing
put the fallback shape through the validator, and tightening `any` to
`all` killed the capability with the whole file still green. Measured: 73
passed under that mutation.
One line puts the same shape through the validator. Now 73 passed
unmutated, 1 failed under the mutation, failing on the fallback assertion
rather than incidentally on a dead shape another test already covers.
Found by Patch76 review on #2256.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012d29UJTiH4Uy2Pm37SBPtr
---------
Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>1 parent ed59e22 commit 43a192c
14 files changed
Lines changed: 2428 additions & 90 deletions
File tree
- src/ha_mcp
- client
- tools
- tests/src
- e2e/workflows
- hacs
- integrations
- unit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
46 | 46 | | |
47 | 47 | | |
48 | 48 | | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
49 | 56 | | |
50 | 57 | | |
51 | 58 | | |
| |||
621 | 628 | | |
622 | 629 | | |
623 | 630 | | |
624 | | - | |
625 | | - | |
626 | | - | |
627 | | - | |
| 631 | + | |
| 632 | + | |
| 633 | + | |
| 634 | + | |
| 635 | + | |
628 | 636 | | |
629 | 637 | | |
630 | 638 | | |
| |||
644 | 652 | | |
645 | 653 | | |
646 | 654 | | |
647 | | - | |
| 655 | + | |
648 | 656 | | |
649 | 657 | | |
650 | 658 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
168 | 168 | | |
169 | 169 | | |
170 | 170 | | |
171 | | - | |
172 | | - | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
173 | 175 | | |
174 | 176 | | |
175 | 177 | | |
| |||
205 | 207 | | |
206 | 208 | | |
207 | 209 | | |
| 210 | + | |
208 | 211 | | |
209 | 212 | | |
210 | 213 | | |
| |||
289 | 292 | | |
290 | 293 | | |
291 | 294 | | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
292 | 306 | | |
293 | 307 | | |
294 | 308 | | |
| |||
332 | 346 | | |
333 | 347 | | |
334 | 348 | | |
| 349 | + | |
335 | 350 | | |
336 | 351 | | |
337 | 352 | | |
| |||
0 commit comments