fix: persist cleared override fields in the in-process server options flow (#1834)

* fix: persist cleared override fields in the in-process server options flow

The server entry's options flow pre-filled its optional text fields
(pip-spec / server-URL / external-URL / webhook-id / secret-path) with a
voluptuous schema `default` equal to the saved value. HA's frontend drops
an emptied optional field from the submitted payload, so the flow manager
re-applied that default and clearing never stuck: the "Developer: ha-mcp
package override" field kept re-installing the old build and re-appeared
populated on reopen. Typing the default dist name ("ha-mcp") was the only
way to clear it, because `_normalize` collapses the default to empty.

Pre-fill via `description={"suggested_value": ...}` instead. It shows the
same value but is not re-injected on an empty submit, so a cleared field
stays cleared while an untouched field is still submitted unchanged.

`ha_dev_manage_server("update_source")` drives the same flow with a partial
submit and relied on those defaults to preserve the fields it does not
change. With the defaults gone, resend the user's current overrides
(reading `description.suggested_value` first) so a channel or pip-spec
change no longer blanks the server URL or connect secrets.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix: address review — normalize server_url, doc accuracy, CodeQL, tests

- config_flow: normalize server_url in _normalize (strip + trim trailing slash,
  drop when blank) so a whitespace-only Home Assistant URL can't be stored
  verbatim and bypass the empty->loopback fallback (Gemini review).
- config_flow: correct the stale _normalize docstring (the pip-spec field is
  pre-filled with the saved override or blank, not DEFAULT_PIP_SPEC) and the
  misquoted "Leave empty" help text.
- tools_dev: fix the _field_prefill / find_server_config_entry docstrings (the
  component sets suggested_value directly; note the value fallback), shorten the
  update_source comment, and log a rejected deferred self-restart at warning.
- tests: split pop() out of the assert in test_form_prefills_* to clear the
  CodeQL py/side-effect-in-assert findings; add server_url whitespace / trailing-
  slash tests and a dev-tool test that a caller pip_spec wins over the preserved
  pin.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: kingpanther13

custom_components/ha_mcp_tools/config_flow.py

@@ -262,16 +262,27 @@ async def async_step_init(
                 ),
                 vol.Optional(
                     OPT_PIP_SPEC,
-                    # Pre-fill only a genuinely saved override. The normalized
-                    # "no override" state renders an EMPTY field — pre-filling
-                    # DEFAULT_PIP_SPEC as a hint made a field whose help text
-                    # says "leave blank" always look populated, and showed the
-                    # STABLE dist name even on the dev channel.
-                    default=opts.get(OPT_PIP_SPEC, ""),
+                    # Pre-fill via suggested_value, NOT a schema default: a
+                    # default equal to the saved value makes the field
+                    # impossible to clear. HA's frontend drops an emptied
+                    # optional field from the submitted payload, so voluptuous
+                    # re-applies the default (the old override) and clearing
+                    # never sticks. suggested_value pre-fills the same value but
+                    # is not re-injected on an empty submit. (Applies to every
+                    # optional text field below.) Only a genuinely saved
+                    # override is suggested; the normalized "no override" state
+                    # renders an EMPTY field — the help text says "Leave empty",
+                    # and pre-filling DEFAULT_PIP_SPEC would show the STABLE dist
+                    # name even on the dev channel.
+                    description={"suggested_value": opts.get(OPT_PIP_SPEC, "")},
                 ): str,
                 vol.Optional(
                     OPT_SERVER_URL,
-                    default=opts.get(OPT_SERVER_URL, DEFAULT_LOOPBACK_URL),
+                    description={
+                        "suggested_value": opts.get(
+                            OPT_SERVER_URL, DEFAULT_LOOPBACK_URL
+                        )
+                    },
                 ): str,
                 vol.Required(
                     OPT_ENABLE_WEBHOOK,
@@ -301,17 +312,23 @@ async def async_step_init(
                         mode=SelectSelectorMode.DROPDOWN,
                     )
                 ),
+                # suggested_value (not default) so these clear properly on an
+                # empty submit — see the OPT_PIP_SPEC note above.
                 vol.Optional(
                     OPT_EXTERNAL_URL,
-                    default=opts.get(OPT_EXTERNAL_URL, ""),
+                    description={"suggested_value": opts.get(OPT_EXTERNAL_URL, "")},
                 ): str,
                 vol.Optional(
                     OPT_WEBHOOK_ID_OVERRIDE,
-                    default=opts.get(OPT_WEBHOOK_ID_OVERRIDE, ""),
+                    description={
+                        "suggested_value": opts.get(OPT_WEBHOOK_ID_OVERRIDE, "")
+                    },
                 ): str,
                 vol.Optional(
                     OPT_SECRET_PATH_OVERRIDE,
-                    default=opts.get(OPT_SECRET_PATH_OVERRIDE, ""),
+                    description={
+                        "suggested_value": opts.get(OPT_SECRET_PATH_OVERRIDE, "")
+                    },
                 ): str,
                 vol.Optional(
                     OPT_REGENERATE_SECRETS,
@@ -343,14 +360,16 @@ async def async_step_init(
 
     @staticmethod
     def _normalize(user_input: dict[str, Any]) -> dict[str, Any]:
-        """Collapse the default pip spec to empty so it is not stored as an override.
-
-        The pip-spec field is pre-filled with ``DEFAULT_PIP_SPEC`` (the unpinned
-        ``ha-mcp`` distribution) as a hint. Persisting that value verbatim would
-        read as an intentional override and disable the stable channel's
-        automatic updates. Collapsing "equals the default" (or empty) to empty
-        keeps the entry tracking the selected channel; a genuine override (any
-        other string) is stored as-is.
+        """Normalize the submitted options before they are persisted.
+
+        Collapses the pip-spec field to empty when it is empty or equals
+        ``DEFAULT_PIP_SPEC`` (the unpinned ``ha-mcp`` distribution): the field is
+        pre-filled with the saved override or blank, but a user may also type the
+        default dist name, and persisting it verbatim would read as an
+        intentional override and disable the stable channel's automatic updates.
+        Empty means "no override" (track the selected channel); any other string
+        is a genuine override, stored as-is. Also strips the URL / secret
+        override fields, and drops a blank ``server_url`` so its default applies.
         """
         cleaned = dict(user_input)
         if cleaned.get(OPT_PIP_SPEC, "").strip() in ("", DEFAULT_PIP_SPEC):
@@ -362,6 +381,15 @@ def _normalize(user_input: dict[str, Any]) -> dict[str, Any]:
         ):
             cleaned[key] = str(cleaned.get(key, "") or "").strip()
         cleaned[OPT_EXTERNAL_URL] = cleaned[OPT_EXTERNAL_URL].rstrip("/")
+        # server_url gets no _normalize-forced empty like the fields above; strip
+        # it and drop it entirely when blank so a whitespace-only value can't be
+        # stored verbatim (it would bypass the consumer's empty -> loopback
+        # fallback and break the HA connection).
+        server_url = str(cleaned.get(OPT_SERVER_URL, "") or "").strip().rstrip("/")
+        if server_url:
+            cleaned[OPT_SERVER_URL] = server_url
+        else:
+            cleaned.pop(OPT_SERVER_URL, None)
         return cleaned
 
     async def _versions_hint(self) -> str:

src/ha_mcp/tools/tools_dev.py

@@ -44,8 +44,25 @@
 # (custom_components/ha_mcp_tools/const.py OPT_CHANNEL / OPT_PIP_SPEC).
 _OPT_CHANNEL = "channel"
 _OPT_PIP_SPEC = "pip_spec"
+_OPT_SERVER_URL = "server_url"
+_OPT_EXTERNAL_URL = "external_url"
+_OPT_WEBHOOK_ID_OVERRIDE = "webhook_id_override"
+_OPT_SECRET_PATH_OVERRIDE = "secret_path_override"
 _VALID_CHANNELS = ("stable", "dev")
 
+# Optional text fields the component's options flow pre-fills via
+# suggested_value (so the UI can clear them). Because an OMITTED optional field
+# reads as "cleared" rather than "unchanged", a partial update_source submit
+# must resend these at their current values or it would blank the user's
+# server-URL / connect-secret overrides.
+_PRESERVED_OPTION_KEYS = (
+    _OPT_PIP_SPEC,
+    _OPT_SERVER_URL,
+    _OPT_EXTERNAL_URL,
+    _OPT_WEBHOOK_ID_OVERRIDE,
+    _OPT_SECRET_PATH_OVERRIDE,
+)
+
 # Delay before a self-affecting action (embedded entry reload / options
 # submit) fires, so this tool's JSON response flushes to the MCP client
 # before the serving thread is torn down. Mirrors
@@ -85,6 +102,23 @@ def _spawn_background(coro: Any) -> None:
     task.add_done_callback(_BACKGROUND_TASKS.discard)
 
 
+def _field_prefill(item: dict[str, Any]) -> Any:
+    """Return a serialized options-flow field's current value.
+
+    Reads ``description.suggested_value`` first: a persisted option is
+    serialized there (as ``add_suggested_values_to_schema`` does; this component
+    sets it directly on the ``vol.Optional`` marker), and the clearable text
+    fields carry their value there rather than as a schema ``default`` (a
+    ``default`` equal to the value would make the field impossible to clear).
+    Falls back to ``default`` then ``value`` for the dropdown/toggle fields.
+    Mirrors ``tools_integrations.options_from_form_flow``.
+    """
+    description = item.get("description")
+    if isinstance(description, dict) and description.get("suggested_value") is not None:
+        return description["suggested_value"]
+    return item.get("default", item.get("value"))
+
+
 async def find_server_config_entry(
     client: Any,
 ) -> tuple[str, dict[str, Any], dict[str, Any]] | None:
@@ -95,8 +129,9 @@ async def find_server_config_entry(
     (services) entry's flow aborts immediately. Returns
     ``(entry_id, open_flow, current_options)`` with the options flow left
     OPEN (callers must submit or abort it), or ``None`` when no server
-    entry exists. ``current_options`` maps schema field names to their
-    defaults — i.e. the entry's current option values.
+    entry exists. ``current_options`` maps schema field names to their current
+    values (persisted ``suggested_value`` first, else the schema ``default`` or
+    ``value``, via ``_field_prefill``).
 
     Module-level (not a DevTools method) so the settings UI's embedded
     restart handler can share it.
@@ -131,7 +166,7 @@ async def find_server_config_entry(
             continue
         schema = flow.get("data_schema") or []
         fields: dict[str, Any] = {
-            str(item["name"]): item.get("default")
+            str(item["name"]): _field_prefill(item)
             for item in schema
             if isinstance(item, dict) and item.get("name")
         }
@@ -400,7 +435,16 @@ async def _delayed_submit_options(
         await asyncio.sleep(_SELF_ACTION_FLUSH_DELAY_S)
         try:
             result = await self._client.submit_options_flow_step(flow_id, user_input)
-            logger.info("Deferred options submit result: %s", result.get("type"))
+            if result.get("type") == "create_entry":
+                logger.info("Deferred options submit applied")
+            else:
+                # Fire-and-forget: no caller is left to raise to, so a rejected
+                # self-restart must at least be discoverable in the log.
+                logger.warning(
+                    "Deferred options submit was not applied (type=%s, errors=%s)",
+                    result.get("type"),
+                    result.get("errors") or result.get("reason"),
+                )
         except Exception:
             logger.exception("Deferred options-flow submit failed")
 
@@ -819,7 +863,12 @@ async def _update_source(
                 )
             )
         entry_id, flow, current = found
-        user_input: dict[str, Any] = {}
+        # Resend the user's current overrides (see _PRESERVED_OPTION_KEYS) so a
+        # channel/pip-spec change here does not blank them — an omitted optional
+        # field reads as "cleared", not "unchanged".
+        user_input: dict[str, Any] = {
+            key: current[key] for key in _PRESERVED_OPTION_KEYS if current.get(key)
+        }
         if channel is not None:
             user_input[_OPT_CHANNEL] = channel
         if pip_spec is not None:

tests/src/unit/test_config_flow.py

@@ -307,7 +307,10 @@ def test_auto_update_defaults_on(self):
     def test_form_prefills_every_field_from_saved_options(self):
         # Review gap: the form must show the user's SAVED values, not the
         # defaults, for every field (a regression here silently reverts a
-        # user's config on the next save).
+        # user's config on the next save). Dropdowns/toggles pre-fill via the
+        # schema default; the optional text fields pre-fill via suggested_value
+        # (a default there would make them impossible to clear — see
+        # test_clearing_an_override_field_sticks).
         saved = {
             const.OPT_CHANNEL: const.CHANNEL_DEV,
             const.OPT_AUTO_UPDATE: False,
@@ -328,10 +331,27 @@ def test_form_prefills_every_field_from_saved_options(self):
             data={const.DATA_WEBHOOK_ID: "mcp_abc"}, options=saved
         )
         form = asyncio.run(flow.async_step_init(None))
-        defaults = {m.schema: m.default() for m in form["data_schema"].schema}
+        markers = {m.schema: m for m in form["data_schema"].schema}
+
+        # Optional text fields pre-fill via suggested_value so they stay
+        # clearable; every other field pre-fills via the schema default.
+        text_fields = (
+            const.OPT_PIP_SPEC,
+            const.OPT_SERVER_URL,
+            const.OPT_EXTERNAL_URL,
+            const.OPT_WEBHOOK_ID_OVERRIDE,
+            const.OPT_SECRET_PATH_OVERRIDE,
+        )
+        for key in text_fields:
+            assert markers[key].description["suggested_value"] == saved[key]
+
+        defaults = {
+            key: m.default() for key, m in markers.items() if key not in text_fields
+        }
         # regenerate_secrets is a one-shot action, never pre-filled True;
         # enable_webhook / enable_startup_notification / enable_sidebar_panel
-        # default on when unsaved.
+        # default on when unsaved. Pop off the schema (not inside assert, which
+        # `python -O` would strip) before comparing the remainder.
         regenerate_default = defaults.pop(const.OPT_REGENERATE_SECRETS)
         assert regenerate_default is False
         webhook_default = defaults.pop(const.OPT_ENABLE_WEBHOOK)
@@ -340,7 +360,7 @@ def test_form_prefills_every_field_from_saved_options(self):
         assert notification_default is True
         panel_default = defaults.pop(const.OPT_ENABLE_SIDEBAR_PANEL)
         assert panel_default is True
-        assert defaults == saved
+        assert defaults == {k: v for k, v in saved.items() if k not in text_fields}
 
     def test_init_submit_round_trips_input_into_entry(self):
         flow = _make_options_flow()
@@ -380,6 +400,31 @@ def test_default_pip_spec_normalized_to_empty(self):
         )
         assert result["data"][const.OPT_PIP_SPEC] == ""
 
+    def test_server_url_whitespace_is_dropped_to_default(self):
+        # A whitespace-only Home Assistant URL must not be stored verbatim: it is
+        # truthy, so it would bypass the consumer's empty -> loopback fallback and
+        # break the connection. _normalize drops it so the default applies.
+        flow = _make_options_flow()
+        result = asyncio.run(
+            flow.async_step_init(
+                {const.OPT_CHANNEL: const.CHANNEL_STABLE, const.OPT_SERVER_URL: "   "}
+            )
+        )
+        assert const.OPT_SERVER_URL not in result["data"]
+
+    def test_server_url_trailing_slash_stripped(self):
+        # A real URL is kept, with any trailing slash trimmed.
+        flow = _make_options_flow()
+        result = asyncio.run(
+            flow.async_step_init(
+                {
+                    const.OPT_CHANNEL: const.CHANNEL_STABLE,
+                    const.OPT_SERVER_URL: "http://ha.local:8123/",
+                }
+            )
+        )
+        assert result["data"][const.OPT_SERVER_URL] == "http://ha.local:8123"
+
     def test_pip_spec_field_empty_when_no_override(self):
         # The "leave blank to follow the channel" field must actually BE
         # blank when no override is stored — pre-filling the default dist
@@ -391,7 +436,40 @@ def test_pip_spec_field_empty_when_no_override(self):
         marker = next(
             m for m in form["data_schema"].schema if m.schema == const.OPT_PIP_SPEC
         )
-        assert marker.default() == ""
+        assert marker.description["suggested_value"] == ""
+
+    def test_clearing_an_override_field_sticks(self):
+        # Regression: emptying an optional text field must persist as cleared.
+        # HA's frontend DROPS an emptied optional field from the submitted
+        # payload; the flow manager then validates that payload against the
+        # shown schema (filling voluptuous defaults) before the step handler
+        # runs — the layer a direct-handler unit test skips. A schema
+        # ``default=<saved value>`` silently re-injects the old value there, so
+        # clearing never took: the pip-spec override kept re-installing the old
+        # build and the field re-appeared populated on reopen. Pre-filling with
+        # ``suggested_value`` (not ``default``) has no such re-injection, so the
+        # cleared state sticks.
+        clearable = {
+            const.OPT_PIP_SPEC: "ha-mcp @ https://example/x.tgz",
+            const.OPT_SERVER_URL: "https://ha.example:8123",
+            const.OPT_EXTERNAL_URL: "https://ha.example.com",
+            const.OPT_WEBHOOK_ID_OVERRIDE: "my_custom_hook",
+            const.OPT_SECRET_PATH_OVERRIDE: "/custom_path",
+        }
+        for field in clearable:
+            flow = _make_options_flow(
+                data={const.DATA_WEBHOOK_ID: "mcp_abc"}, options=dict(clearable)
+            )
+            form = asyncio.run(flow.async_step_init(None))
+            # The user cleared exactly one field; the frontend omits it and
+            # submits the rest. Validate through the shown schema exactly as the
+            # flow manager does, then hand the result to the step.
+            submitted = {k: v for k, v in clearable.items() if k != field}
+            validated = form["data_schema"](submitted)
+            result = asyncio.run(flow.async_step_init(validated))
+            assert result["data"].get(field, "") == "", (
+                f"clearing {field!r} did not persist: {result['data'].get(field)!r}"
+            )
 
     def test_no_enable_toggle_option_exists(self):
         # Regression guard for the single-instance pivot: the enable/disable