fix: reject blank locale catalog values instead of rendering them (#2163)

* fix: reject blank locale catalog values instead of rendering them

English is the per-key fallback only for a key that is ABSENT. `t()` and
`tHtml()` resolve with `hasOwnProperty` rather than truthiness, so a
present-but-empty string wins over the English source and renders as nothing at
all. Omitting a key and emptying it therefore look nearly identical in the JSON
and behave oppositely on screen — a translator clearing a value to mean "not
translated yet" silently blanks that piece of UI.

Nothing objected to it. `_validate_string_map` and `_validate_tools` checked the
type and stopped, and the parity ceilings count a key untranslated only when it
equals the English or is missing (`_untranslated_keys`), so `""` read there as
translated. Placeholder parity caught the subset whose English carries a
placeholder, which is why this stayed invisible: the loud half of the class was
already covered.

Both validators now reject a blank value the way `meta.native_name` already
does, and the error names the fix — omit the key. Measured before changing it:
0 blank values across 4,985 shipped ones (messages, tool_groups, tools and the
component catalogs), so this rejects nothing that exists today.

Each guard is pinned separately: breaking the `messages`/`tool_groups`
condition fails those six parametrizations while the two tool-field cases stay
green, and breaking the tool-field condition inverts that exactly.

The locale README gains the rule where it describes omission, since omitting is
now the only way to express "not translated yet".

* docs: state the blank rule on tool_groups without promising omission

`_validate_string_map` guards `messages` and `tool_groups` alike, but the
catalog reference stated the blank rule only under `messages`, so a
translator reading the `tool_groups` bullet had no reason to expect a load
error.

Omission is not the way out on that section: the key set is exact, and
`test_settings_catalog_keys_name_real_groups_and_tools` — gated behind the
post-merge `LOCALE_COMPLETENESS_CHECKS=1` run — fails a catalog that is
missing a group. An untranslated heading therefore keeps the English tag as
its value.

For the same reason the shared blank-value error names the mechanism, that
English renders only for an absent key, rather than prescribing "omit the
key": that advice holds for `messages` and points a `tool_groups` translator
at a catalog which then fails the completeness check.

The per-tool blank test now covers the same three blank spellings as its
`messages`/`tool_groups` sibling instead of two spaces alone.

`_style_sample_keys` claimed a hand-committed blank was "only type-checked"
by `_validate_string_map`, which this branch made false. Its defensive skip
is still worth keeping, so the docstring now names the reason that actually
holds: the script reads catalogs with `json.loads` instead of going through
`load_catalogs`, so a blank reaches it out of a tree the app never loaded.

* docs: name why the sampler still meets a blank the loader now rejects

`_style_sample_keys` and the test that pins it both justified the blank-skip
with `_validate_string_map` doing no more than a type check. The previous
commit makes that false.

The skip is still worth keeping, so both now name the reason that does hold:
`scripts/translate_locales.py` reads catalogs with `json.loads` rather than
through `load_catalogs`, so a blank reaches the sampler out of a working tree
the app never loaded.

Author: Patch76

scripts/translate_locales.py

@@ -465,10 +465,13 @@ def _style_sample_keys(
     right-hand side into the prompt, so the pair it contributes shows the model
     nothing to imitate while still counting against the sample budget — and on
     a catalog whose register rests on a single key, that is the whole signal.
-    Nothing upstream stops it reaching here: an engine answer this shape is
-    rejected (`_validate`), but a hand-committed one is only type-checked
-    (`_validate_string_map`), and the parity ceilings count a key untranslated
-    only when it equals the English or is absent, so `""` reads as translated.
+    An engine answer this shape is rejected (`_validate`), and a hand-committed
+    one no longer survives a catalog load (`_validate_string_map`) — but this
+    script reads the catalogs with `json.loads` rather than through
+    `load_catalogs`, so one still reaches here out of a working tree the app has
+    not loaded. The parity ceilings would not object either: they count a key
+    untranslated only when it equals the English or is absent, so `""` reads
+    there as translated.
     """
     return sorted(
         (

src/ha_mcp/settings_ui/_i18n.py

@@ -23,7 +23,18 @@
 
 
 def _validate_string_map(value: Any, *, context: str) -> dict[str, str]:
-    """Return a validated ``str -> str`` catalog section."""
+    """Return a validated ``str -> str`` catalog section.
+
+    A blank value is rejected rather than stored. English is the per-key
+    fallback, but only for a key that is ABSENT: ``t()`` and ``tHtml()`` pick
+    the catalog value with ``hasOwnProperty``, so a present-but-empty string
+    wins over English and renders as nothing at all. Omitting a key and
+    emptying it therefore look identical in the JSON and behave oppositely on
+    screen, which is the kind of difference no reviewer catches by reading.
+    Nothing else covers it either — the parity ceilings count a key
+    untranslated only when it equals the English or is missing, so ``""``
+    reads there as translated.
+    """
     if value is None:
         return {}
     if not isinstance(value, dict):
@@ -32,6 +43,12 @@ def _validate_string_map(value: Any, *, context: str) -> dict[str, str]:
     for key, text in value.items():
         if not isinstance(key, str) or not isinstance(text, str):
             raise ValueError(f"{context} must contain only string keys and values")
+        if not text.strip():
+            raise ValueError(
+                f"{context}.{key} is blank — English renders only for an "
+                "absent key, so translate it, or leave the key out where the "
+                "section allows a missing one"
+            )
         result[key] = text
     return result
 
@@ -53,6 +70,13 @@ def _validate_tools(value: Any, *, context: str) -> dict[str, dict[str, str]]:
                 continue
             if not isinstance(field_value, str):
                 raise ValueError(f"{context}.{tool_name}.{field} must be a string")
+            # Same rule as _validate_string_map: blank loses to nothing, an
+            # omitted field falls back to the tool's own English metadata.
+            if not field_value.strip():
+                raise ValueError(
+                    f"{context}.{tool_name}.{field} is blank — omit the field "
+                    "instead, so the English tool metadata renders"
+                )
             translated[field] = field_value
         unknown = set(tool_values) - {"title", "description"}
         if unknown:

src/ha_mcp/settings_ui/locales/README.md

@@ -61,13 +61,19 @@ or constructed — needs no pipeline change.
   rejected when the catalog loads.
 - `messages`: interface labels, help text, notices, and runtime messages. Keys
   may be omitted — English is the per-key fallback at runtime — but see the
-  share limit below before leaving a catalog half-finished.
+  share limit below before leaving a catalog half-finished. Omitting is the
+  only way to say "not translated yet": a key that is present but blank is
+  rejected when the catalog loads, because the runtime resolves by key
+  presence, so an empty value would win over English and render as nothing.
 - `tool_groups`: one entry per renderable MCP tool tag, keyed by the English
-  tag. Not optional, and exact: no key more and none fewer.
+  tag. Not optional, and exact: no key more and none fewer. Blank is rejected
+  here too, but dropping the key is not the escape hatch it is for `messages` —
+  the exact key set forbids that. A heading you have not translated yet keeps
+  the English tag as its value.
 - `tools`: `title` and `description` per tool, keyed by the stable MCP tool
   name. The key set is not optional and exact in the same way; either field on
   its own may be left out, but a missing one counts as untranslated against the
-  share limit below.
+  share limit below. Blank is rejected here too, for the same reason.
 
 Keep the keys and `{placeholders}` unchanged in every section.
 

tests/src/unit/test_settings_ui_i18n.py

@@ -230,6 +230,55 @@ def test_catalog_without_a_native_name_is_rejected(tmp_path: Path) -> None:
         load_catalogs(tmp_path)
 
 
+@pytest.mark.parametrize("blank", ["", " ", "\n\t "])
+@pytest.mark.parametrize("section", ["messages", "tool_groups"])
+def test_blank_catalog_value_is_rejected(
+    tmp_path: Path, section: str, blank: str
+) -> None:
+    """Blank and absent look the same in JSON and behave oppositely on screen.
+
+    English is the per-key fallback only for a key that is ABSENT: `t()` and
+    `tHtml()` resolve with `hasOwnProperty`, so an empty string wins over the
+    English source and renders as nothing. A translator emptying a value to
+    mean "not translated yet" would silently blank that piece of UI, and no
+    other check objects — the parity ceilings count a key untranslated only
+    when it equals the English or is missing.
+    """
+    catalog = {
+        "meta": {"native_name": "English", "dir": "ltr"},
+        "messages": {},
+        "tool_groups": {},
+        "tools": {},
+    }
+    catalog[section] = {"a": blank}
+    (tmp_path / "en.json").write_text(json.dumps(catalog), encoding="utf-8")
+
+    with pytest.raises(ValueError, match=re.escape(f"en.json.{section}.a is blank")):
+        load_catalogs(tmp_path)
+
+
+@pytest.mark.parametrize("blank", ["", " ", "\n\t "])
+@pytest.mark.parametrize("field", ["title", "description"])
+def test_blank_tool_field_is_rejected(tmp_path: Path, field: str, blank: str) -> None:
+    """Same rule on the per-tool surface, which validates separately."""
+    (tmp_path / "en.json").write_text(
+        json.dumps(
+            {
+                "meta": {"native_name": "English", "dir": "ltr"},
+                "messages": {},
+                "tool_groups": {},
+                "tools": {"ha_get_state": {field: blank}},
+            }
+        ),
+        encoding="utf-8",
+    )
+
+    with pytest.raises(
+        ValueError, match=re.escape(f"en.json.tools.ha_get_state.{field} is blank")
+    ):
+        load_catalogs(tmp_path)
+
+
 def test_inline_payload_escapes_script_breakout() -> None:
     serialized = serialize_payload({"messages": {"unsafe": "</script><b>&"}})
 

tests/src/unit/test_translate_locales.py

@@ -162,8 +162,9 @@ def test_skips_keys_the_catalog_has_not_translated(self) -> None:
 
     @pytest.mark.parametrize("blank", ["", " ", "\n\t "])
     def test_skips_keys_whose_translation_is_blank(self, blank: str) -> None:
-        """A blank value is a present key with nothing in it, and the sampler is
-        the only thing that looks: `_validate_string_map` type-checks, and the
+        """A blank value is a present key with nothing in it, and this sampler
+        is where one still turns up: `_validate_string_map` rejects it at load,
+        but this script reads the catalogs with `json.loads` instead, and the
         parity ceilings count a key untranslated only when it equals the English
         or is missing, so `""` reads there as translated. Sampled, it spends one
         of three slots on a pair whose target side is empty — and a catalog