fix(settings): homeassistant-ai#1431 review pass — address 13 verified findings (homeassistant-ai#1164)

Round-3 review pass landed 13 verified findings; the cosmetic
"persisted-value visible in UI after F5" item was explicitly skipped
per user direction (UI shows post-gate value when master off; user
accepts this since beta tools are actually disabled at runtime — the
preserve-across-master-cycle UX is at the data layer, not the visual).

Source fixes:

- **Save button copy is now source-blind** — the previous "your
  feature-flag toggles already saved on click" claim only held when
  ``saveFeatureFlag`` raised ``restartNotice``; tool-config pin saves,
  backup-config saves, and cross-tab ``restart-required`` broadcasts
  also raise it. New copy: "a restart is pending. Click Restart above
  to apply your prior changes." (#2)
- **Stale F.37 test docstring** describing the deleted server-side
  cascade rewritten. (#3)
- **Beta-gate INFO log noise** — cascade-clear removal meant the gate
  could fire its "forcing %s=False" line every Settings rebuild,
  spamming addon logs once a user had truthy sub-flags persisted.
  Dedup via ``_BETA_GATE_LOGGED`` set per process, cleared on
  ``_reset_global_settings``. (#9)
- **Lazy-lock docstring** updated to reflect Python 3.13 semantics
  (``asyncio.Lock()`` no longer takes a loop arg; the lazy pattern
  still serves test fixtures and single-loop deployment, with the
  invariant documented). (#10)
- **Addon-mode carve-out comment** clarified to distinguish dev
  (master in schema) from stable (master web-UI-only). (homeassistant-ai#14)
- **probe-div null branch** in F.37 now writes ``data-error`` so a
  failing test points at "selector missed" vs "value flipped"
  unambiguously. (homeassistant-ai#17)

Tests added:

- ``test_translations_cover_every_schema_key`` — parity check that
  every ``schema:`` key has a non-empty translation ``name`` and
  ``description``. Parameterised across stable + dev addons. Pins
  the class of silent gap that this PR's ``advanced_debug_logging``
  fix addressed. (#4)
- ``test_save_features_acquires_override_file_lock`` +
  ``test_save_advanced_acquires_override_file_lock`` — counting-lock
  wrapper asserts ``async with _get_override_file_lock()`` runs
  exactly once in each file-mode write path. Pin against a regression
  that silently bypasses concurrent-save serialisation. (#5)
- ``test_dual_save_buttons_mirror_disabled_and_status_on_post_failure``
  — exercises the 500-response branch of the dual-save mirror so a
  regression that broke ``_setAdvSaveStatus``/``_setAdvSaveDisabled``
  for error paths only would still fail. (#6)
- ``test_save_features_master_on_restores_subflag_values_in_addon_mode``
  — addon-mode round-trip mirror of the existing standalone restore
  test; asserts the Supervisor merge-and-post call carries only the
  master flip-on and never zeroes out sub-flag values. (#7)
- ``test_save_button_nothing_to_save_when_no_dirty_and_no_restart`` +
  ``test_save_button_restart_pending_hint_when_dirty_empty_but_restart_showing``
  — both branches of the empty-dirty Save click are exercised; the
  restart-pending branch asserts the copy is source-blind. (#8)

Deferred per user direction:
- #1 (file-vs-Settings visual after F5): user accepts the current
  behavior (UI shows post-gate value; runtime tools actually
  disabled when master off; data-layer preserve still works
  end-to-end).
- #11/#12/homeassistant-ai#13 (code-simplifier helper extractions): skipped as
  complicated to implement without behavior risk.
- homeassistant-ai#15 (pre-homeassistant-ai#1164 users with already-cleared sub-flags): release-note
  concern, not a code change.
- homeassistant-ai#16 (lock fragility under future thread-pool dispatch):
  speculative future-risk; not actionable today.

Author: kingpanther13

src/ha_mcp/config.py

@@ -814,12 +814,20 @@ def _apply_feature_flag_overrides(settings: "Settings") -> None:
                 )
                 continue
             current = getattr(settings, sub, False)
-            if current:
+            if current and sub not in _BETA_GATE_LOGGED:
+                # Dedup per-process: cascade-clear was removed so the
+                # file now holds truthy sub-flag values long-term and
+                # this gate runs on every Settings rebuild. Logging
+                # the force-False line every time would spam addon
+                # logs (#1164 follow-up review). First-time-per-process
+                # is enough to leave an audit trail for operators
+                # debugging "why is my beta tool off?".
                 logger.info(
                     "Beta master toggle is off; forcing %s=False "
                     "(was True via env/file).",
                     sub,
                 )
+                _BETA_GATE_LOGGED.add(sub)
             try:
                 setattr(settings, sub, False)
             except (ValueError, TypeError) as err:
@@ -954,6 +962,14 @@ def _apply_advanced_overrides(settings: "Settings") -> None:
 # Global settings instance
 _settings: Settings | None = None
 
+# Names of beta sub-flags the master gate has already logged a
+# force-False line for in this process. Used to dedup the gate's
+# INFO log so we don't spam addon logs on every Settings rebuild
+# now that the cascade-clear is gone and the file may carry truthy
+# sub-flag values long-term (#1164 follow-up review). Reset alongside
+# the Settings singleton in ``_reset_global_settings``.
+_BETA_GATE_LOGGED: set[str] = set()
+
 
 # Auto-backup runtime-editable fields (#1288 web UI editor). Each entry
 # is (field_name, env_var_name, python_type). The web UI's
@@ -1231,6 +1247,11 @@ def _reset_global_settings() -> None:
     """
     global _settings
     _settings = None
+    # Drop the gate-log dedup set too — once Settings has been
+    # rebuilt, an operator who's re-investigating "why is my beta
+    # tool off?" should see the next gate fire logged. This keeps
+    # the dedup tight to the lifetime of one cached Settings.
+    _BETA_GATE_LOGGED.clear()
 
 
 # Import-time validator for cross-registry invariants (#1164).

src/ha_mcp/settings_ui.py

@@ -3547,18 +3547,17 @@ def apply_tool_visibility(
   if (Object.keys(_advancedDirty).length === 0) {
     // Feature-flag toggles (master beta, Tool Search, etc.) auto-save
     // on click via ``saveFeatureFlag`` — they don't pass through
-    // ``_advancedDirty``. If a feature-flag save just landed,
-    // ``restartNotice`` is showing and the user should click Restart,
-    // not Save again. Tell them that explicitly so the big Save
-    // button doesn't look broken when they were toggling beta flags
-    // (#1164 follow-up).
+    // ``_advancedDirty``. Tool-config pins and backup-config edits
+    // also auto-save. Any of those raise ``restartNotice``, but this
+    // tab can't tell which one. Keep the hint source-blind: just
+    // point at the Restart button (#1164 follow-up review).
     const restartNotice = document.getElementById('restartNotice');
     const restartShowing =
       restartNotice && restartNotice.classList.contains('show');
     if (restartShowing) {
       _setAdvSaveStatus(
-        'No advanced changes to save — your feature-flag toggles already ' +
-        'saved on click. Click Restart above to apply them.'
+        'No advanced changes to save — a restart is pending. Click ' +
+        'Restart above to apply your prior changes.'
       );
     } else {
       _setAdvSaveStatus('Nothing to save.');
@@ -3945,10 +3944,15 @@ async def _supervisor_merge_and_post_options(
 
 
 def _get_override_file_lock() -> asyncio.Lock:
-    """Lazy lock construction — ``asyncio.Lock()`` at module load
-    binds the event loop that's current AT IMPORT, which doesn't exist
-    yet for handlers invoked under uvicorn/starlette. Construct on
-    first use under the live loop instead.
+    """Lazy lock construction. ``asyncio.Lock()`` on Python 3.10+ no
+    longer takes a ``loop=`` argument and only binds to a loop on
+    first ``acquire()`` via ``asyncio.get_event_loop()``. Either eager
+    or lazy module-level construction works for this project's
+    single-uvicorn-loop deployment; we keep the lazy pattern so a
+    future test fixture that spins up its own loop doesn't lock in
+    the import-time loop. Assumes a single asyncio event loop for the
+    process lifetime — a multi-loop deployment (e.g. threaded handler
+    dispatch) would race here.
     """
     global _OVERRIDE_FILE_LOCK
     if _OVERRIDE_FILE_LOCK is None:
@@ -4466,8 +4470,13 @@ async def _save_feature_flags(request: Request) -> JSONResponse:
         # Applied in BOTH standalone and addon mode (#1164 follow-up).
         # The earlier "skip in addon mode" carve-out existed because
         # start.py used to auto-write ENABLE_BETA_FEATURES=true from
-        # any beta sub-flag presence; now start.py writes the master
-        # env from its own options key, so the gate applies uniformly.
+        # any beta sub-flag presence; that path is now demoted to a
+        # one-cycle legacy bridge. On dev addon, start.py writes the
+        # master env var from the schema-bound options key. On stable
+        # addon, the master is not in schema and the standalone
+        # web-UI master path remains the gate (the gate read below
+        # falls through to the override-file value). Either way the
+        # gate is sound to apply uniformly.
         from .config import (
             BETA_FEATURE_FIELDS as _BETA_SUB,
         )

tests/addon/test_addon_structure.py

@@ -45,44 +45,58 @@ def test_config_yaml_valid(self):
         with open("pyproject.toml", "rb") as f:
             pyproject = tomllib.load(f)
         expected_version = pyproject["project"]["version"]
-        assert config["version"] == expected_version, \
+        assert config["version"] == expected_version, (
             f"Add-on version {config['version']} should match package version {expected_version}"
+        )
 
         # Verify essential configurations
         assert config["hassio_api"] is True, "hassio_api required for Supervisor"
         assert config["homeassistant_api"] is True, "homeassistant_api required"
 
         # Verify image field uses per-architecture naming
-        assert config["image"] == "ghcr.io/homeassistant-ai/ha-mcp-addon-{arch}", \
+        assert config["image"] == "ghcr.io/homeassistant-ai/ha-mcp-addon-{arch}", (
             "image field must use per-architecture naming with {arch} placeholder"
+        )
 
         # Verify port configuration (fixed internal port)
         assert "ports" in config, "ports section required for HTTP transport"
         assert "9583/tcp" in config["ports"], "port 9583/tcp must be exposed"
 
         # Verify secret_path configuration (optional advanced override)
-        assert "secret_path" not in config["options"], \
+        assert "secret_path" not in config["options"], (
             "secret_path should be optional and omitted so Supervisor treats it as advanced"
-        assert "secret_path" in config["schema"], "schema must include secret_path field"
-        assert config["schema"]["secret_path"] == "str?", \
+        )
+        assert "secret_path" in config["schema"], (
+            "schema must include secret_path field"
+        )
+        assert config["schema"]["secret_path"] == "str?", (
             "secret_path schema should be optional string (str?)"
+        )
 
         # Verify backup_hint configuration
-        assert "backup_hint" in config["options"], "options must include backup_hint field"
-        assert config["options"]["backup_hint"] == "normal", "default backup_hint should be normal"
-        assert config["schema"]["backup_hint"] == "list(strong|normal|weak|auto)", \
+        assert "backup_hint" in config["options"], (
+            "options must include backup_hint field"
+        )
+        assert config["options"]["backup_hint"] == "normal", (
+            "default backup_hint should be normal"
+        )
+        assert config["schema"]["backup_hint"] == "list(strong|normal|weak|auto)", (
             "backup_hint schema must enumerate allowed values"
+        )
 
         # Verify architectures (only 64-bit platforms supported by uv image)
         expected_archs = ["amd64", "aarch64"]
         assert all(arch in config["arch"] for arch in expected_archs)
 
         # Verify 32-bit platforms are not included
         unsupported_archs = ["armhf", "armv7", "i386"]
-        assert not any(arch in config["arch"] for arch in unsupported_archs), \
+        assert not any(arch in config["arch"] for arch in unsupported_archs), (
             "32-bit platforms not supported by uv base image"
+        )
 
-    @pytest.mark.skipif(sys.platform == "win32", reason="Unix permissions not applicable on Windows")
+    @pytest.mark.skipif(
+        sys.platform == "win32", reason="Unix permissions not applicable on Windows"
+    )
     def test_start_script_executable(self):
         """Verify start.py has executable permissions."""
         start_py = f"{ADDON_DIR}/start.py"
@@ -95,3 +109,43 @@ def test_start_script_has_shebang(self):
             first_line = f.readline()
         assert first_line.startswith("#!"), "start.py missing shebang"
         assert "python" in first_line.lower(), "start.py shebang must reference python"
+
+    @pytest.mark.parametrize(
+        "addon_dir", ["homeassistant-addon", "homeassistant-addon-dev"]
+    )
+    def test_translations_cover_every_schema_key(self, addon_dir):
+        """Every key declared in ``config.yaml``'s ``schema:`` must have a
+        matching ``configuration.<key>`` entry in ``translations/en.yaml``
+        with both ``name`` and ``description`` populated. Pre-#1164 the
+        ``advanced_debug_logging`` schema field was added on stable but
+        the translation was forgotten — the addon Configuration UI
+        then showed an unlabelled checkbox. Lock the parity so the
+        same class of silent gap can't recur.
+        """
+        with open(f"{addon_dir}/config.yaml") as f:
+            cfg = yaml.safe_load(f)
+        with open(f"{addon_dir}/translations/en.yaml") as f:
+            translations = yaml.safe_load(f)
+        schema_keys = set(cfg.get("schema", {}).keys())
+        # ``secret_path`` is intentionally undocumented in user-facing
+        # translations (it's an advanced/hidden override the wizard
+        # handles, not a user-set option).
+        schema_keys.discard("secret_path")
+        configuration = translations.get("configuration", {})
+        for key in sorted(schema_keys):
+            entry = configuration.get(key)
+            assert entry is not None, (
+                f"{addon_dir}/translations/en.yaml is missing a "
+                f"`configuration.{key}` entry for the schema field "
+                f"declared in config.yaml"
+            )
+            assert entry.get("name"), (
+                f"{addon_dir}/translations/en.yaml `configuration.{key}` "
+                "needs a non-empty `name` (Supervisor renders it as the "
+                "user-facing toggle label)"
+            )
+            assert entry.get("description"), (
+                f"{addon_dir}/translations/en.yaml `configuration.{key}` "
+                "needs a non-empty `description` (Supervisor renders it "
+                "as the help tooltip under the toggle)"
+            )

tests/src/unit/test_settings_ui.py

@@ -2928,6 +2928,174 @@ async def test_save_features_payload_master_false_sub_true_rejected_by_gate(
         get_data_dir.cache_clear()
         _reset_global_settings()
 
+    @pytest.mark.asyncio
+    async def test_save_features_acquires_override_file_lock(
+        self, monkeypatch, tmp_path
+    ):
+        """F.5 — pin the lock-acquire site so a regression that removes
+        ``async with _get_override_file_lock():`` from the file-mode
+        write path lands as a test failure. The lock is the only thing
+        preventing two concurrent saves from clobbering each other's
+        persisted state, but the protection is invisible to other
+        tests — they'd still pass without it.
+        """
+        from ha_mcp.config import FEATURE_FLAG_FIELDS, _reset_global_settings
+        from ha_mcp.settings_ui import build_settings_handlers
+        from ha_mcp.utils.data_paths import get_data_dir
+
+        get_data_dir.cache_clear()
+        monkeypatch.setenv("HA_MCP_CONFIG_DIR", str(tmp_path))
+        get_data_dir.cache_clear()
+        for _fname, ename, _ftype in FEATURE_FLAG_FIELDS:
+            monkeypatch.delenv(ename, raising=False)
+        monkeypatch.delenv("SUPERVISOR_TOKEN", raising=False)
+        # Wrap _get_override_file_lock so we can count entries.
+        import ha_mcp.settings_ui as ui_mod
+
+        real_get_lock = ui_mod._get_override_file_lock
+        entries = {"count": 0}
+
+        class CountingLock:
+            def __init__(self, inner):
+                self._inner = inner
+
+            async def __aenter__(self):
+                entries["count"] += 1
+                return await self._inner.__aenter__()
+
+            async def __aexit__(self, *args):
+                return await self._inner.__aexit__(*args)
+
+        def patched_get_lock():
+            return CountingLock(real_get_lock())
+
+        monkeypatch.setattr(ui_mod, "_get_override_file_lock", patched_get_lock)
+        _reset_global_settings()
+        handlers = build_settings_handlers(server=None)
+        req = MagicMock()
+        req.json = AsyncMock(return_value={"flags": {"enable_tool_search": True}})
+        resp = await handlers["save_feature_flags"](req)
+        assert resp.status_code == 200, json.loads(resp.body)
+        assert entries["count"] == 1, (
+            "save_feature_flags must acquire _OVERRIDE_FILE_LOCK exactly once "
+            "in the file-mode write path — regression would silently bypass "
+            "concurrent-save serialisation"
+        )
+        get_data_dir.cache_clear()
+        _reset_global_settings()
+
+    @pytest.mark.asyncio
+    async def test_save_advanced_acquires_override_file_lock(
+        self, monkeypatch, tmp_path
+    ):
+        """F.5 — same lock-acquire pin for the advanced-settings file
+        write path. Both handlers must share the same lock so they
+        can't race against each other on the shared
+        ``feature_flags.json``.
+        """
+        from ha_mcp.config import _reset_global_settings
+        from ha_mcp.settings_ui import build_settings_handlers
+        from ha_mcp.utils.data_paths import get_data_dir
+
+        get_data_dir.cache_clear()
+        monkeypatch.setenv("HA_MCP_CONFIG_DIR", str(tmp_path))
+        monkeypatch.delenv("HA_TIMEOUT", raising=False)
+        monkeypatch.delenv("SUPERVISOR_TOKEN", raising=False)
+        get_data_dir.cache_clear()
+        import ha_mcp.settings_ui as ui_mod
+
+        real_get_lock = ui_mod._get_override_file_lock
+        entries = {"count": 0}
+
+        class CountingLock:
+            def __init__(self, inner):
+                self._inner = inner
+
+            async def __aenter__(self):
+                entries["count"] += 1
+                return await self._inner.__aenter__()
+
+            async def __aexit__(self, *args):
+                return await self._inner.__aexit__(*args)
+
+        def patched_get_lock():
+            return CountingLock(real_get_lock())
+
+        monkeypatch.setattr(ui_mod, "_get_override_file_lock", patched_get_lock)
+        _reset_global_settings()
+        handlers = build_settings_handlers(server=None)
+        req = MagicMock()
+        req.json = AsyncMock(return_value={"timeout": 90})
+        resp = await handlers["save_advanced_settings"](req)
+        assert resp.status_code == 200, json.loads(resp.body)
+        assert entries["count"] == 1, (
+            "save_advanced_settings must acquire _OVERRIDE_FILE_LOCK in the "
+            "file-mode write path — regression would silently bypass "
+            "concurrent-save serialisation"
+        )
+        get_data_dir.cache_clear()
+        _reset_global_settings()
+
+    @pytest.mark.asyncio
+    async def test_save_features_master_on_restores_subflag_values_in_addon_mode(
+        self, monkeypatch, tmp_path
+    ):
+        """F.7 — round-trip restore is exercised in standalone mode by
+        ``test_save_features_master_on_restores_runtime_subflag_values``.
+        Mirror for addon mode: the Supervisor merge-and-post must NOT
+        zero out sub-flag values when the user POSTs only the master
+        flip-on. The preserve-on-master-off → restore-on-master-on UX
+        only works in addon mode if Supervisor keeps the merged options
+        intact.
+        """
+        from ha_mcp.config import _reset_global_settings
+        from ha_mcp.settings_ui import build_settings_handlers
+
+        monkeypatch.setenv("SUPERVISOR_TOKEN", "fake")
+        # Dev-addon options.json shape: master + sub-flag both true,
+        # then user flipped master off (sub-flag value preserved).
+        # Supervisor's current options state at fetch time reflects
+        # that shape.
+        current_options = {
+            "enable_beta_features": False,
+            "enable_yaml_config_editing": True,
+            "enable_filesystem_tools": True,
+            "backup_hint": "normal",
+        }
+        # Mock the fetch+post path so we can inspect what gets POSTed.
+        fetch_mock = AsyncMock(return_value=(current_options, None))
+        merge_mock = AsyncMock(return_value=(True, None))
+        monkeypatch.setattr(
+            "ha_mcp.settings_ui._supervisor_fetch_current_options", fetch_mock
+        )
+        monkeypatch.setattr(
+            "ha_mcp.settings_ui._supervisor_merge_and_post_options",
+            merge_mock,
+        )
+        # Mark the master env var as set so get_feature_flag_origin
+        # returns 'addon' for it and the save handler picks the
+        # addon-route branch.
+        monkeypatch.setenv("ENABLE_BETA_FEATURES", "false")
+        _reset_global_settings()
+        server = MagicMock()
+        server.settings.verify_ssl = True
+        handlers = build_settings_handlers(server=server)
+        req = MagicMock()
+        req.json = AsyncMock(return_value={"flags": {"enable_beta_features": True}})
+        resp = await handlers["save_feature_flags"](req)
+        assert resp.status_code == 200, json.loads(resp.body)
+        # The Supervisor POST got only the master flip. Sub-flag
+        # values are NOT in the call — Supervisor's merge layer
+        # preserves the existing options.json entries for keys not
+        # mentioned in the POST body.
+        merge_mock.assert_awaited_once()
+        posted_args = merge_mock.await_args.args
+        assert posted_args[1] == {"enable_beta_features": True}, (
+            f"addon-route POST must NOT clobber sub-flag values; "
+            f"posted: {posted_args[1]}"
+        )
+        _reset_global_settings()
+
     @pytest.mark.asyncio
     async def test_save_features_master_off_applied_dict_contains_only_master(
         self, monkeypatch, tmp_path