Commit f193d96
feat: reconfigure existing Home Assistant integrations safely (#2149)
* feat: add generic integration reconfigure tool
* fix: harden generic integration reconfigure
* fix: close reconfigure review blockers
* feat: expose honest integration rollback metadata
* fix: preserve rollback guidance after verification failure
* fix: allow auxiliary entries sharing a device
* fix: address reconfigure review findings
* feat: harden generic integration reconfigure flow
* fix: allow auxiliary entries sharing a device
* docs: align maintainer guidance
* test: block reconfigure when registry transport fails
* fix: keep offline reconfigure verification conservative
* refactor: remove reconfigure response redaction
* fix: classify degraded reconfigure outcomes
* fix: harden reconfigure identity and confirmation
* test: align reconfigure e2e with confirmation token
* docs: describe reconfigure preflight confirmation
* refactor: unify reconfigure preflight state
* fix: validate reconfigure registry identity
* fix: retry transient reconfigure verification
* fix: enforce post-reconfigure identity anchors
* fix: abort flows on caller cancellation
* chore: remove generated docs from reconfigure change
* fix: reject reconfigure without identity anchor
* fix: preserve prepared reconfigure identity anchors
* fix: correct the reconfigure safety layer's blocking, status and retry rules
Addresses the round-2 review on #2149.
Blocking and identity:
- Block only on a SAME-domain entry sharing the device. HA routinely attaches
helper platforms (utility_meter, derivative, statistics, history_stats,
trend, threshold, integration, switch_as_x) to the source device, and
blocking on those made every entry carrying one unreconfigurable — in the
read-only preflight too, with no override. Cross-domain relations are now
reported as warnings; an unresolvable domain still blocks, fail-closed.
- Replace the identity bag with a `ReconfigureIdentity` dataclass and return
the related-entry classification as a frozen `RelatedEntries` instead of
writing it back through an argument the signature calls an input. The
write-only `registry_available` flags are gone.
- Delete the three post-commit `expected_*` checks that could never fire
(pre-flow forces expected == before, the changed-guard forces after ==
before). The MAC twin is the only reachable one; it now has tests.
Status vocabulary:
- Type the seven outcomes as `ReconfigureStatus`, replacing bare strings
across three modules, and add `applied_identity_mismatch` so a device-swap
safety violation is no longer flattened into "registry read timed out".
- Gate `flow_budget_exhausted` on reconfigure mode: it changed the public
error shape of the add-integration, options and helper flows for a field
only the reconfigure path reads.
- Report integration failures in integration vocabulary, not "Helper
validation failed".
Verification:
- `async_update_reload_and_abort` schedules the reload and returns, so the
read-back landed mid-reload and a reconfigure that WORKED reported
`applied_but_unverified`. Retry while the entry state is transitional
(not_loaded / setup_in_progress, unless disabled) before settling, and make
the message agree with the final status.
- Registry reads no longer degrade to "unavailable": they return a list or
raise, so an empty list means "the entry has none", never "we could not
tell". Read them through the component layer with the whole-registry dump
as the fallback leg, and share one config-entry list-all per verification.
- Drop the verification fields that were constants or mislabelled.
Also: `_reject_redaction_sentinels` now guards the reconfigure path, the
fourth surface writing caller config into a flow — without it a caller
round-tripping a redacted read would write the placeholder over a live
credential. And the post-commit failure context no longer uses the key
`error`, which `create_error_response` merges over the structured error
block, taking the code and suggestions with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* refactor(internal): split the reconfigure surface out of tools_integrations
`tools_integrations.py` had grown past 3,500 lines and this PR added a
further coherent concern to it. The reconfigure surface — the parameters
legal only in that mode, the confirm-token handshake, the read-only preflight
response and the runner — moves to `integration_reconfigure.py`; the tool
itself stays put and delegates. Test patch targets move with it.
Deliberately not named `tools_*`: the registry auto-discovers that prefix
expecting a `register_*_tools` function, which `test_tool_module_naming`
enforces.
The same commit settles the API the review objected to:
- Drop `confirm`. `confirm_token` alone is the handshake, matching
`ha_config_set_yaml` — which also removes the silent dead state where
`confirm=False` with a token returned another preview and discarded it.
- `ignored_parameters` -> `rejected_parameters`: the call is rejected and
nothing ran, so "ignored" told a parsing agent the opposite of what
happened. The list is now declared once in `RECONFIGURE_ONLY_PARAMETERS`.
- The preflight says what it actually checked (entry exists, supports
reconfigure, identity anchors, no duplicate) and that config keys are NOT
validated until confirm; it no longer carries both `status: "preview"` and
`preview: true`.
- One name for the rollback metadata (`rollback`), not three.
- Drop the "This is not Shelly-specific" rebuttal, one-line the EXAMPLES
entry that shipped multi-line in the model-facing description, document the
reconfigure caveats and the `supports_reconfigure` discovery route, and say
on each `expected_*` that it requires `reconfigure=True`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* test: cover the reconfigure paths the review found unverified
- Give the e2e harness a reconfigure-capable target. None of the nine seeded
entries implemented `async_step_reconfigure`, so both reconfigure e2e tests
skipped forever and the confirmed path had zero repeatable coverage. Seeds
a `filesize` entry (core, single `file_path` field, commits through
`async_update_reload_and_abort` — the exact call whose scheduled reload the
retry loop has to outlast) plus two files under `www/`, and replaces the
env-var opt-in with four real tests that use `MCPAssertions`.
- Stub the registry reads in the unit client doubles. ~14 tests ran against a
bare `MagicMock()`, whose non-awaitable return was read as "registry
unavailable", so they silently skipped the identity and duplicate logic
they existed to cover — and asserted a result the real client cannot
produce.
- Replace the personal-install fixtures (real entity names, MACs and
10.0.50.x addresses) with neutral ones.
- Fix the retry test: its `side_effect` list ran dry, so attempts 2 and 3
raised `StopAsyncIteration("")` and the real cause never reached the
operator while the test still passed. Adds the recovery leg it was missing.
- Add the untested branches: flow start without a `flow_id`, the MCP-tool
error wrapper, undrivable and unexpected step types, ieee/zigbee satisfying
`expected_mac` while `upnp` does not, malformed device rows, the subentry
committed-flow and create_entry siblings, and both `reconfigure` mode
guards.
- Finish the half-done `ToolError` import sweep in test_flow_multistep, drop
a duplicate test, and restore the deleted `# === Mode delegation ===`
header.
- Repin `ha_set_integration.description` in the locale baseline: this branch
edits that docstring, and `test_a_pending_hidden_rendering_keeps_its_base_key_checked`
(added in #2180, after this PR's last CI run) fails until it moves.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* test: pin the confirm token to the change it previewed
Staleness was only exercised by mutating the entry title. The property that
protects the caller is that the token is bound to the requested config and
the supplied identity anchors, so neither can be swapped under an old token.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: reject a malformed confirm token instead of erroring internally
hmac.compare_digest raises TypeError on a non-ASCII str operand, so a
caller's junk token reached the outer handler and came back as
INTERNAL_ERROR rather than the stale-preflight rejection. Compares encoded
bytes now, with a regression test.
Also corrects the walker docstring, which claimed the reconfigure success
shape carries operation "reconfigure" where the code deliberately emits the
past-tense "reconfigured" that set_config_subentry reads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: expose unique_id from ha_get_integration and harden the reconfigure e2e
The e2e caught two things on its first real CI run.
ha_get_integration never returned unique_id, so ha_set_integration's
expected_unique_id had no discovery route — a caller could not read the
anchor it is meant to assert. Added to _format_entry with unit coverage,
including the None case for entries that have none (MQTT).
The confirmed-apply e2e also assumed a starting file_path and an ordering.
filesize's reconfigure step calls _abort_if_unique_id_configured(), which
does NOT exclude the entry being reconfigured, so reconfiguring to the path
the entry already holds aborts with already_configured. It now reads the
current path, targets the other one, and restores in a finally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* test: assert a field Home Assistant actually returns
Reverts the _format_entry unique_id addition and the two unit tests behind
it. HA does not expose a config entry's unique_id: ConfigEntry.as_json_fragment
omits it, and every config-entry endpoint (REST list, WS config_entries/get,
get_single) serializes through that fragment. The field would have been
structurally None forever, and the tests only passed because they fed
_format_entry a dict HA never produces.
The e2e now asserts `title`, which is in the fragment and which filesize
rewrites on reconfigure, and picks its target from the current title so it
stays order- and rerun-independent.
This does NOT fix the underlying gap it exposed - see the PR discussion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* feat: make expected_unique_id actually work, via the custom component
The e2e run against a live instance exposed that the whole unique_id half of
the reconfigure identity layer was dead. Home Assistant withholds a config
entry's unique_id from every endpoint it has — `ConfigEntry.as_json_fragment`
carries no such key, and the REST list, `config_entries/get` and `get_single`
all serialize that fragment — so `entry.get("unique_id")` was `None` for every
real entry. `expected_unique_id` always failed with "the registries report
none", the before/after guard never fired, and `unique_id_preserved` was
always True because it compared two Nones. Unit tests missed it because every
fixture hand-built an entry dict WITH a unique_id, a shape HA never sends.
The component holds the live ConfigEntry, so it can supply the value. Its
`config_entries` row now carries `unique_id` as a deliberate superset of
core's fragment (the row-shape test already documented the superset contract).
Deployment split, which is the point of the design:
- With the custom component: the anchor works. The value feeds the pre-flow
check, the post-commit before/after guard, the duplicate scan and the
confirm token.
- Without it (add-on / Docker / PyPI): the value is UNREADABLE, which is not
the same as absent. `ReconfigureIdentity.unique_id_known` keeps the three
states apart, so verification reports
`unique_id_verification: "unavailable_without_component"` and
`unique_id_preserved: null` rather than claiming preservation, and a
supplied `expected_unique_id` is rejected with the real reason and a pointer
to the anchors that do work there.
- Component present but the entry genuinely has none (MQTT): known-absent, and
"preserved" is then a true statement.
No version gate and no component bump: the field is additive within
schema_version 1, and an older component omits the KEY, which is
distinguishable from sending it as None. Same discipline as `device_get`'s
opt-in entities join. It rides the existing pending 1.4.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: treat a malformed component reply as unknown, not known-absent
CodeRabbit caught two real defects in the new component read.
A non-dict response from ws.send_command raised AttributeError out of a
helper documented as never raising. And a non-string unique_id was coerced to
None with known=True — reporting "this entry has no unique_id" for junk,
which is precisely the conflation the three-state type exists to prevent and
would have silently disarmed the anchor.
Both now degrade to UNKNOWN_UNIQUE_ID, along with a row whose entry_id does
not match the one asked for (reading another entry's anchor is worse than
reading none). New test module covers every branch: value, known-absent, no
capability, pre-field row, non-string value, wrong entry, six malformed
payload shapes, caps invalidation on unknown_command, and three transport
failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: allow a legitimate unique_id re-key, block only its loss
CI's e2e caught the guard I had just enabled being too strict. filesize keys
its config entry on the file path, so reconfiguring the path re-keys the
unique_id through async_update_reload_and_abort(unique_id=...) — a supported
HA pattern, not a safety violation. The guard refused it.
The hazard the maintainer named is LOSING the unique_id: the entry becomes
indistinguishable to discovery, which then creates a duplicate. So a clear
(set -> None) still raises; a change to a different value is reported as
unique_id_verification: "changed_during_change" and left to the caller.
That also makes expected_unique_id genuinely enforceable post-commit, which is
the point of the anchor: a caller who needs the value pinned across a
reconfigure says so, and a re-key is then refused. The check I had removed as
unreachable is reachable now that a re-key is permitted, so it is back, and
the docstring says which of the four checks can fire and why.
Also fixes RUF036 (None not last in a type union) in the test helper. It
passed my local ruff 0.15.15 and failed CI's pinned 0.16.1 — the lint must be
run through uv, not the host binary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: close four identity holes the Codex review found
All four verified against the code before fixing; none were taken on trust.
1. The confirm token did not bind the DISCOVERED identity. It hashed the
entry's unique_id and the caller's expected_* anchors — but most callers
supply none and rely on what the preview showed them, so the device,
entity and MAC associations moving between preview and confirm left the
token valid and applied against an identity the caller never approved.
The token now covers the discovered sets.
2. A pinned expected_unique_id that could not be re-read after the commit was
treated as passed. `comparable` went false and the guard was skipped, so an
unchanged device/entity set could still report identity_verification
"complete" and applied_and_verified. It now reports
unique_id_verification "anchor_unverifiable_after_change" and degrades to
applied_but_unverified. Deliberately unverified rather than failed:
nothing is known to be wrong, it simply could not be checked.
3. The duplicate scan compared unique_ids that its rows never carry.
`list_config_entries()` returns as_json_fragment rows, which have no
unique_id, so a same-domain duplicate not sharing a device was undetectable
in production while the response still claimed
duplicate_scan "unique_id_and_shared_device". The unit test masked it by
injecting unique_id into mocked REST rows. The scan now reads the domain's
unique_ids through the component in ONE call, and reports
"shared_device_only" when that is unavailable rather than claiming a check
it did not make.
4. A disabled entry could never verify. Its terminal state is not_loaded —
_is_transient_reconfigure_state already says so — but
operational_state_verified demanded "loaded", so every clean reconfigure of
a disabled entry came back applied_but_unverified.
Also documents the e2e backend-lane markers in tests/AGENTS.md, including the
distinction between the ha_mcp_tools COMPONENT (installed on every lane) and
its in-process server config entry (embedded lanes only).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: forward verify_ssl on component reads and fail the domain map closed
Three CodeRabbit findings, each verified against the code first.
**verify_ssl was dropped on every component read.** The capability probe
forwards `getattr(client, "verify_ssl", None)` — with a comment saying why —
but the reads did not, and verify_ssl keys the client pool. A client with
verify_ssl=False therefore passed detection on one pooled client and then
failed the read on another, degrading identity verification for no real
reason. CodeRabbit flagged it on the new module; it was the same in three
siblings that predate this PR, so all seven call sites are swept
(component_config_entries, component_devices, component_registry_lookup,
component_registries) per the Boy Scout rule.
**The domain unique_id map could be incomplete but look complete.** It dropped
non-dict rows and rows with a bad entry_id or non-string unique_id, then
returned a map the duplicate scan reads as a COMPLETED check — so a malformed
duplicate row could be silently excluded and let verification pass. Any
malformed row now returns None for the whole map: incomplete must be
indistinguishable from unreadable. A genuinely None unique_id remains the one
valid omission.
**tests/AGENTS.md described external_only wrongly** — and so does the
conftest docstring I took it from, which is where the error came from. The
marker skips inaddon / container-embedded / HAOS-embedded and RUNS on plain
testcontainer plus HAOS external; #1375 already found 14 supervisor-mock
tests silently skipping because of that same misreading. Corrected in both
places, with a note to read the skip expression rather than the summary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: observe the post-commit reload instead of polling for it
Closes the last hole Patch76 found, plus his two coverage gaps and the
verify_ssl sweep.
**Airtight reload verification.** Home Assistant queues the post-reconfigure
reload with `hass.async_create_task` and returns (`async_schedule_reload`,
core `config_entries.py`), changing no state synchronously. So a read-back can
land in either direction: mid-reload — already handled — or BEFORE the reload
starts, sampling the stale pre-reload `loaded` and settling as
`applied_and_verified` while a failing reload goes unseen. Polling cannot tell
a finished reload from one that has not begun.
It is no longer polled. `ConfigEntry._async_set_state` dispatches
SIGNAL_CONFIG_ENTRY_CHANGED on EVERY state change, which
`config_entries/subscribe` forwards with the full fragment, so the
reconfigure now opens that stream BEFORE the flow and reads the entry's
settled state off it. `subscribe_command` registers the queue before sending,
so no transition between the commit and the first read can be missed — an
observed state is evidence the reload actually reached it. The stream is torn
down under `asyncio.shield`, mirroring the `hacs/subscribe` precedent. When no
stream can be opened the old retry loop still runs, and the response now says
which mechanism produced the answer via `operational_state_source`
(`observed` / `polled`) rather than leaving the caller to guess.
**A lost submit answer no longer escapes the taxonomy.** `_submit_step`
special-cased only `TimeoutError`, but rest_client funnels every httpx
transport failure — ConnectError, TimeoutException, HTTPError — into
`HomeAssistantConnectionError`, so an ordinary connection reset after the POST
reached HA propagated raw with no status AND had its flow aborted by the
generic handler, despite that flow possibly having committed. A caller reads a
bare connection error as "nothing happened" and retries. It now raises
`applied_but_unverified` for every class in `_NO_ANSWER_ERRORS` — the
vocabulary rest_client already documents as "no answer came back" — which also
suppresses the wrong abort via POST_COMMIT_STATUSES.
**Two unpinned guards.** His mutation pass found the post-commit same-domain
shared-device branch and both arms of the entry identity guard were unpinned;
mutating them left the suite green. Both now have tests, verified by
reproducing his mutations and watching them go red.
**verify_ssl swept repo-wide** — all 18 remaining call sites across 15 files.
`verify_ssl` keys the client pool, so omitting it hands back a different
pooled client than the capability probe used: detection passes, the read then
fails against a self-signed cert. Zero omissions remain.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: settle the reload observation on a state change, not any fragment
Patch76 found the observation mechanism failing in the direction it was
built to prevent. Verified every claim against HA core at both `dev` and the
`2024.11.0` floor in hacs.json before changing anything; all of them hold.
**Three fragments beat the reload to the queue, and two of them predate it.**
`config_entries/subscribe` answers with a snapshot of every current entry
(`[{"type": None, "entry": ...}]`) and `subscribe_command` registers the queue
before sending, so that snapshot is queued rather than dropped. Committing the
new values then dispatches `ConfigEntryChange.UPDATED` with the new data and
the OLD state, before `async_schedule_reload` runs. Settling on either reported
the pre-reload `loaded` as an observed result — strictly worse than the poll it
replaced, because a non-None observation also switches off the transient-state
retry. Neither `type` nor `modified_at` can order these: every state change
dispatches the same UPDATED, and the commit bumps `modified_at` itself.
So the criterion is a state CHANGE. The first fragment sets a baseline and only
a departure from it can settle. The commit fragment carries the same state as
the baseline and is ignored on exactly that basis.
**`unload_in_progress` was terminal, and it is the first state a reload
reaches.** `ConfigEntry.async_unload` sets it before calling the component, so
the observation settled on sight and reported a good reload as unverified. It
also reached the polled path. Absent at 2024.11.0, present on dev — so this
broke only on current Home Assistant. Now in the transient set, which both
paths read.
**A disabled entry no longer burns the settle budget.** `async_unload` returns
early at `not_loaded` without setting state and `async_reload` skips setup
while `disabled_by` is set, so no transition is ever coming. The snapshot says
so up front; hand back to the poll immediately instead of stalling 20s.
**The same ambiguity was still escaping as a 5xx.** `_submit_step` maps every
`_NO_ANSWER_ERRORS` class to `applied_but_unverified`, but the
`HomeAssistantAPIError` branch above it re-raised anything that is not 400/422
with no status, so it reached the generic handler and aborted a
possibly-committed flow — the abort `POST_COMMIT_STATUSES` exists to suppress.
A 504 from a proxy in front of Home Assistant is the ordinary way there. 5xx is
now the no-answer line: 4xx means HA parsed the request and answered about it
(rejection, auth, unknown flow), which is not the post-commit ambiguity.
**And a 504 is no longer replayed on a write.** `_RETRYABLE_STATUS` retried
502/503/504 up to three times on the premise its own comment states — "the
upstream couldn't be reached, so the request did not execute and retrying is
safe even for writes". That holds for 502/503 and not for 504, where the
upstream WAS reached and merely answered too slowly, so a write may already
have executed. The reconfigure submit is exactly such a write. 504 now retries
only for safe methods; 502/503 keep retrying for all.
Every new test verified to bite by reproducing the mutation it pins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: never gateway-retry the config-flow submit
CodeRabbit is right that 502/503 do not prove a write never executed — RFC 9110
has 502 as "received an invalid response from an inbound server", which a proxy
only sends after reaching it. The comment claiming otherwise was wrong and is
corrected.
Its proposed fix — stop retrying 502/503 for all unsafe methods — is declined:
that retry exists because an HA-restart window otherwise surfaced
SERVICE_CALL_FAILED on ordinary writes and flaked the in-addon E2E suite, and
it covers 6 POST and 4 DELETE call sites. Taxing every write for the narrow
case trades a real, observed failure for a rarer one.
The real hole is narrower and sits in this PR's own path. `submit_config_flow_step`
is retried like any other POST, so a submit that commits and then loses its
answer as a 502 gets replayed — and Home Assistant has consumed the flow_id by
then, so the replay returns 404 "Invalid flow specified"
(`helpers/data_entry_flow.py`). That 4xx reads as a definitive answer, does not
reach the new 5xx branch, and the generic handler aborts the flow: the caller
sees "invalid flow" and concludes nothing happened. Precisely the ambiguity
`applied_but_unverified` exists to name, resurrected by the retry.
So the opt-out is per request rather than global: `_request` /`_raw_request`
take `retry_gateway_errors`, and the submit is the one caller that passes
False. Every other write keeps its restart-window resilience unchanged.
Comments and docstrings trimmed to the non-obvious per .gemini/styleguide.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: never gateway-retry a write, and split the reconfigure module
**The 502/503 write retry was a bug, not a tradeoff.** I defended it as a
deliberate restart-window decision; that was wrong. The commit that introduced
it (#1623) states its own justification verbatim: "A gateway 5xx means the
request never reached the backend, so retrying is safe even for writes." RFC
9110 contradicts that — 502 is "received an invalid response from an inbound
server", which a proxy only sends after reaching it, and 504 means the upstream
answered too slowly. There was never a weighing of double-apply risk; the write
extension rode entirely on a false premise, so this corrects a bug rather than
trading one failure for another.
What that retry actually bought was a 502 storm failing ~190 tests at once, and
that blast radius is overwhelmingly reads — which keep retrying. Writes now
fail loudly instead of being silently replayed, which is recoverable by the
caller in a way a double-applied write is not: firing an event twice, running a
script twice, or turning a completed DELETE into a misleading 404 on the replay.
DELETE was evaluated for retry as an idempotent method and rejected for exactly
that last reason — replaying it converts a success into a 404, the same failure
shape as the config-flow submit bug. Safe methods only.
This also subsumes the previous commit's per-request opt-out, which is now
redundant: the submit is a POST, so the method rule already covers it. The
comment naming it as the motivating case stays.
**Module split** (CodeRabbit, against the ~1000-line guideline in AGENTS.md):
config_entry_flow.py 1972 -> 1390, with two concerns extracted whole —
`config_entry_identity.py` (registry reads, related-entry classification,
identity comparison) and `config_entry_reload_watch.py` (the subscription and
the settle rule). Test patch targets moved with the code they patch:
`fetch_entities_for_config_entry_via_component`,
`fetch_device_list_via_component` and `fetch_config_entry_unique_id` now resolve
against `config_entry_identity`.
Adds `test_config_entry_reload_watch.py` — 22 tests covering the settle rule
directly rather than only through the full reconfigure path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* refactor: split the reconfigure surface out of config_entry_flow
Every module in the family is now under the ~1000-line guideline:
config_entry_flow.py 1972 -> 448
config_entry_reconfigure.py - 974 (new)
config_entry_identity.py - 510 (earlier in this PR)
integration_reconfigure.py 352
component_config_entries.py 224
config_entry_reload_watch.py 136 (earlier in this PR)
The cut is along the real seam rather than by line count. `config_entry_flow`
creates and updates entries, helpers and subentries; a reconfigure edits a live
entry in place and Home Assistant reloads it afterwards, so nearly all of its
code is about proving the entry still points at the same physical device once
the change has committed. `config_entry_reconfigure` depends one way on
`config_entry_flow` for the shared flow-abort and sentinel-rejection helpers,
never the reverse — the same shape as walker -> form -> menu.
Importers and test patch targets moved with the code they name:
`integration_reconfigure`, `test_tools_integrations` and
`test_integration_reconfigure` now resolve `PreparedReconfigure`,
`reconfigure_config_entry`, `fetch_domain_unique_ids`,
`_subscribe_entry_changes` and `_RELOAD_SETTLE_TIMEOUT` against their new
homes. A patch target left pointing at the old module fails silently — the
stub simply stops being applied — so each was traced rather than assumed.
Also the two CodeRabbit nitpicks on 0f12ad4:
`list_entity_registry` and `list_device_registry` were byte-identical apart
from the command type and the error noun; both now call `_list_registry`.
And the MAC guard's asymmetry is documented rather than "fixed". It requires
BOTH sets to be non-empty, so losing every MAC passes, while the device and
entity guards fire on an emptied set. That is deliberate: MACs come from a
device's registry `connections`, which some integrations re-register with
`identifiers` only after a reload, and a device swap is already caught by the
device guard. Treating an emptied set as a mismatch would raise a POST-COMMIT
identity error on a perfectly good reconfigure. A MAC that changes to a
different value still fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: keep post-commit failures inside the reconfigure taxonomy
Both CodeRabbit findings on 48638b4 are real, and the second is the more
serious of the two.
**A broken change stream escaped past the classifier.** The flow has COMMITTED
by the time `_observe_reload_settled` runs, but only its timeout path was
handled. Anything else propagating out of the queue loop skipped
`_verify_reconfigure_result` entirely and took the rollback metadata and the
`applied_*` status with it, leaving the caller a bare traceback for a change
Home Assistant had already applied — precisely the outcome this vocabulary
exists to prevent. Reaching it needs nothing exotic: `_entry_fragments` calls
`message.get()`, so a single non-dict frame raises `AttributeError`.
The observation now lives in `_observed_reload_state`, which never raises and
degrades to the polled path, logging what went wrong. `CancelledError`
propagates.
**A verification error could be dropped on the floor.** `after` and
`verification` are pinned by the first attempt that succeeds. When that attempt
saw a transient state the loop retries, so a later attempt failing with a
non-`ToolError` left `last_verification_error` set but unreported: the
"nothing succeeded" guard cannot fire, because an earlier attempt did. The
status stayed conservative — that read never reached `loaded` — but the caller
was told the result was unverified without being told why. The last error now
rides along in `warnings`.
Both verified by reproducing the mutation each test pins: dropping the
post-commit guard fails the broken-stream test, dropping the surfaced error
fails the never-settled test.
`reconfigure_config_entry` crossed C901 with the added branches, hence the
helper extraction rather than a per-file ignore.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: attribute the observed reload to the commit, not to any transition
Patch76's third review. Verified both against core at `dev` and the
`2024.11.0` floor before changing anything; both hold.
**A foreign transition inside the flow window still settled the stream.** The
subscription opens before the flow, so the queue holds every fragment from the
whole window. The baseline rule dropped fragments repeating the baseline
state — which covered the snapshot and the commit — but a genuinely foreign
state CHANGE is not a repeat, and a non-transient one settled on sight.
The mainline reconfigure supplies one. An entry in `setup_retry`, the state a
user reconfigures to fix, always has a retry pending while Core runs; one
firing while the flow probes puts `setup_in_progress` and then that retry's
outcome on the stream. Its `loaded` was reported as the reconfigure's result,
with `operational_state_verified=True` and `applied_and_verified`, for a reload
that ended in `setup_retry`. Nothing downstream caught it: the observed
fragment outranks the poll and disarms the transient retry.
`modified_at` is the discriminator, and the docstring was wrong to dismiss it.
`_async_set_state` never touches it, `async_update_entry` bumps it at the
commit, and `as_json_fragment` carries it on both refs. So a fragment still at
the baseline value predates the commit, and the first larger value IS the
commit — which carries the new data and the OLD state, and so cannot settle
either. Only after that point does a state change mean the reload, and by then
no foreign retry can interfere, because `async_schedule_reload` cancels the
pending one before queueing. The claim that `modified_at` cannot order these
was true only of the narrower job of telling the commit from the reload.
On the gap he left to decide: `async_update_entry` returns early on `not
changed`, so resubmitting identical values never bumps `modified_at` and
nothing is ever attributable. Rather than stall the whole settle budget waiting
for a bump that is not coming, `_COMMIT_VISIBLE_TIMEOUT` bounds the
pre-commit phase and hands the answer to the poll, which handles that case
fine.
**And a verified result claimed it never settled.** `last_verification_error`
was never cleared, so a read-back that failed once and then succeeded still
tripped the warning added last push — telling the caller verification never
settled while the status was `applied_and_verified`. Cleared on success.
`_observe_reload_settled` crossed C901 with the added phases, so the queue
handling is now `_EntryFragmentReader`, which also fixes a latent bug: a frame
can carry more than one fragment for the entry (the subscribe snapshot is one
frame holding every entry) and the previous shape could drop the extras.
Each new test verified by reproducing the mutation it pins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* fix: read modified_at in the shape Home Assistant actually sends
Patch76 caught that the commit gate was dead on arrival. `_modified_at`
accepted only `str`, but `as_json_fragment` — what `config_entries/subscribe`
pushes for the snapshot and for every change, and what the REST read returns —
emits `"modified_at": self.modified_at.timestamp()`, a JSON number, on `dev`
and at the `2024.11.0` floor alike. So the parser rejected the very first
fragment, `baseline` was always `None`, `_observe_reload_settled` returned
immediately, and `operational_state_source` was permanently `polled`. The
subscription opened, streamed, and was torn down without ever being read: the
pre-reload-versus-finished-reload ambiguity it exists to remove was back, with
the transient-state retry as the only guard.
The ISO form is real but lives on the other serializer — `as_dict`, which
writes the `.storage` payload — and that is what made the mistake easy: the
storage fixtures carry ISO strings, so it looked like the shape. The repo
already pinned the truth in `_timestamp()` in the component's websocket_api,
whose docstring names `.timestamp()` as a float in seconds.
`_modified_at` now returns epoch seconds and accepts the numeric form, with
`str` kept as tolerance and `bool` excluded (it is an `int` subclass).
The suite could not see any of this, which is the more important half. Both
fixtures defined their timestamps as ISO strings, so three
`operational_state_source == "observed"` assertions passed against a shape the
stream never carries. They are numeric now, and a parametrized test drives the
settle path over float, int and ISO so a str-only regression fails.
The E2E lanes drove a real reconfigure but asserted nothing about which
mechanism answered, so a green run was consistent with the observation never
running. `test_reconfigure_confirmed_applies_and_verifies` now asserts
`operational_state_source == "observed"` — the only assertion in the suite that
sees the real fragment shape.
Verified by reverting the parser to str-only: the shape test and the observed
assertions go red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
* test: make the modified_at rejection path actually pinnable
Patch76 found that the two tests covering the rejection path could not fail.
Each queued a single fragment, so `_observe_reload_settled` returned `None`
either way: the real parser stops at the `baseline is None` check, while a
parser that wrongly ACCEPTS the baseline runs on to the next `reader.next()`,
finds no second fragment, and returns `None` from there. Same assertion, two
unrelated reasons — deleting the `isinstance(raw, bool)` guard left both files
at 149 passed, identical to the control.
The guard is not decorative: `float(True)` is `1.0`, so a `True` baseline makes
`_is_post_commit` accept whatever fragment arrives first and a pre-commit
fragment is read as the commit. Smaller than the shape bug — core emits a float
from `as_json_fragment` and a string from `as_dict`, so nothing at or above the
declared 2024.11.0 floor sends a bool — but unpinned all the same.
Both tests now queue two fragments behind the bad baseline: one carrying a
usable `modified_at` to stand in for the commit, then one whose state differs
and is non-transient to settle on. A single extra fragment is swallowed by the
post-commit loop and never reaches the settle loop, and a settle fragment
repeating the stand-in's state or carrying a transient one leaves the test
green, so both are needed. The settle fragment needs no `modified_at` of its
own, since post-commit fragments are not re-checked.
Verified against both mutations he named. Making every rejection return `0.0`
while leaving numeric and `str` parsing intact reddens all seven params across
the two tests — the property they are there to assert. Deleting only the `bool`
branch reddens exactly the `True` arm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PiqS1JvfcEv7H3XmeXj7Cx
---------
Co-authored-by: LINE-dev <254788708+LINEdev-ipc@users.noreply.github.com>
Co-authored-by: kingpanther13 <25392815+kingpanther13@users.noreply.github.com>
Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 9f56f6d commit f193d96
44 files changed
Lines changed: 8467 additions & 179 deletions
File tree
- custom_components/ha_mcp_tools
- src/ha_mcp
- client
- tools
- tests
- initial_test_state
- .storage
- www
- src
- e2e
- workflows/integrations
- unit
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3316 | 3316 | | |
3317 | 3317 | | |
3318 | 3318 | | |
3319 | | - | |
| 3319 | + | |
3320 | 3320 | | |
3321 | 3321 | | |
3322 | 3322 | | |
3323 | 3323 | | |
3324 | 3324 | | |
3325 | 3325 | | |
3326 | 3326 | | |
3327 | | - | |
| 3327 | + | |
| 3328 | + | |
| 3329 | + | |
3328 | 3330 | | |
3329 | 3331 | | |
3330 | 3332 | | |
| |||
3424 | 3426 | | |
3425 | 3427 | | |
3426 | 3428 | | |
| 3429 | + | |
| 3430 | + | |
| 3431 | + | |
| 3432 | + | |
| 3433 | + | |
| 3434 | + | |
| 3435 | + | |
| 3436 | + | |
| 3437 | + | |
3427 | 3438 | | |
3428 | 3439 | | |
3429 | 3440 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
39 | | - | |
40 | | - | |
41 | | - | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
42 | 54 | | |
43 | 55 | | |
44 | 56 | | |
| |||
304 | 316 | | |
305 | 317 | | |
306 | 318 | | |
307 | | - | |
| 319 | + | |
| 320 | + | |
308 | 321 | | |
309 | 322 | | |
310 | 323 | | |
| |||
323 | 336 | | |
324 | 337 | | |
325 | 338 | | |
326 | | - | |
327 | | - | |
328 | | - | |
329 | | - | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
330 | 344 | | |
331 | 345 | | |
332 | 346 | | |
| |||
1211 | 1225 | | |
1212 | 1226 | | |
1213 | 1227 | | |
| 1228 | + | |
| 1229 | + | |
| 1230 | + | |
| 1231 | + | |
| 1232 | + | |
| 1233 | + | |
| 1234 | + | |
| 1235 | + | |
| 1236 | + | |
| 1237 | + | |
| 1238 | + | |
| 1239 | + | |
| 1240 | + | |
| 1241 | + | |
| 1242 | + | |
| 1243 | + | |
| 1244 | + | |
| 1245 | + | |
| 1246 | + | |
| 1247 | + | |
| 1248 | + | |
| 1249 | + | |
1214 | 1250 | | |
1215 | 1251 | | |
1216 | 1252 | | |
| |||
1228 | 1264 | | |
1229 | 1265 | | |
1230 | 1266 | | |
| 1267 | + | |
| 1268 | + | |
| 1269 | + | |
| 1270 | + | |
1231 | 1271 | | |
1232 | 1272 | | |
1233 | 1273 | | |
| |||
1382 | 1422 | | |
1383 | 1423 | | |
1384 | 1424 | | |
| 1425 | + | |
| 1426 | + | |
| 1427 | + | |
| 1428 | + | |
| 1429 | + | |
| 1430 | + | |
| 1431 | + | |
| 1432 | + | |
| 1433 | + | |
| 1434 | + | |
| 1435 | + | |
| 1436 | + | |
| 1437 | + | |
| 1438 | + | |
| 1439 | + | |
| 1440 | + | |
| 1441 | + | |
| 1442 | + | |
| 1443 | + | |
| 1444 | + | |
| 1445 | + | |
| 1446 | + | |
| 1447 | + | |
| 1448 | + | |
| 1449 | + | |
| 1450 | + | |
| 1451 | + | |
| 1452 | + | |
| 1453 | + | |
| 1454 | + | |
| 1455 | + | |
| 1456 | + | |
| 1457 | + | |
| 1458 | + | |
| 1459 | + | |
| 1460 | + | |
| 1461 | + | |
1385 | 1462 | | |
1386 | 1463 | | |
1387 | 1464 | | |
| |||
1393 | 1470 | | |
1394 | 1471 | | |
1395 | 1472 | | |
1396 | | - | |
| 1473 | + | |
| 1474 | + | |
| 1475 | + | |
1397 | 1476 | | |
1398 | 1477 | | |
1399 | 1478 | | |
1400 | 1479 | | |
1401 | 1480 | | |
1402 | | - | |
1403 | | - | |
1404 | | - | |
1405 | | - | |
1406 | | - | |
1407 | | - | |
1408 | | - | |
1409 | | - | |
1410 | | - | |
1411 | | - | |
1412 | | - | |
| 1481 | + | |
1413 | 1482 | | |
1414 | | - | |
| 1483 | + | |
1415 | 1484 | | |
1416 | 1485 | | |
1417 | 1486 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
255 | 255 | | |
256 | 256 | | |
257 | 257 | | |
258 | | - | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
259 | 263 | | |
260 | 264 | | |
261 | 265 | | |
| |||
0 commit comments