From 73e886dafefe6ebc53fbae77055bcb63021dc63e Mon Sep 17 00:00:00 2001 From: James Mackay Date: Sat, 10 Oct 2026 20:50:16 +0100 Subject: [PATCH] =?UTF-8?q?fix(config=5Fflow):=20F-A/F-A2/F-D=20=E2=80=94?= =?UTF-8?q?=20the=20discovered=20host=20and=20the=205555=20port=20ride=20t?= =?UTF-8?q?he=20schema=20KEY=20defaults=20(t33)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F-A (HIGH): _user_schema_dict re-declared vol.Optional(CONF_ADB_HOST) / CONF_PROTOCOL_HOST inside `if "host" in defaults:` — a no-op, because voluptuous keys compare by field name and the first (defaultless) key object survives. The discovered address therefore never reached the form, and the user submitted a blank host. The host keys are now declared WITH default=host (_host_field_key), so a discovery-driven user step arrives pre-filled. F-A2: a discovery-driven user step says the address is known ("{discovery_note}Discovered Shield at — its address is already filled in below. ") through description_placeholders, and the no_state_source re-render keeps the prefill instead of rebuilding a blank schema. F-D: the ADB port is declared with default=DEFAULT_ADB_PORT (5555), and a YAML seed's port hint is carried into the form instead of being flattened to 5555. The seed host now feeds _seed_host (same prefill, no discovery claim). Reproduce-through-test: tests/voluptuous_mirror.py mirrors the real voluptuous.Marker key contract (hash/eq by field name, default=UNDEFINED when omitted), because voluptuous is not installed in either CI variant and the old bare-key stubs could not tell a schema WITH a default from one without. The pre-t33 discovery assertion only checked the stored attribute and stayed green while the form rendered blank; it now reads the schema keys and fails on the pre-fix code (8 tests reproduced red locally). Verified: uvx --with pytest-asyncio pytest tests -q → 242 passed, 1 skipped; uvx ruff check custom_components tests → clean. --- .../nvidia_shield/config_flow.py | 96 ++++++++++++++----- custom_components/nvidia_shield/strings.json | 2 +- .../nvidia_shield/translations/en.json | 2 +- tests/entity_stublib.py | 14 ++- tests/test_standalone_setup.py | 27 ++++-- tests/voluptuous_mirror.py | 89 +++++++++++++++++ 6 files changed, 193 insertions(+), 37 deletions(-) create mode 100644 tests/voluptuous_mirror.py diff --git a/custom_components/nvidia_shield/config_flow.py b/custom_components/nvidia_shield/config_flow.py index f55a08d..328246e 100644 --- a/custom_components/nvidia_shield/config_flow.py +++ b/custom_components/nvidia_shield/config_flow.py @@ -154,12 +154,31 @@ def _app_map_from_text( return app_map +def _host_field_key(field: str, host: str): + """The host-field KEY, carrying the known address as its default. + + t33 F-A: the default MUST ride on this key. Re-declaring a bare + ``vol.Optional(field)`` afterwards is a no-op that silently drops it — + voluptuous keys compare by field name, so the first (defaultless) key + object survives and the form renders blank. + """ + if host: + return vol.Optional(field, default=host) + return vol.Optional(field) + + def _user_schema_dict(sel, defaults: dict | None = None) -> dict: """User-step schema — the androidtv entity is optional (standalone). - ``defaults`` pre-fills name/hosts from a zeroconf discovery event. + ``defaults`` pre-fills name/hosts from a zeroconf discovery event (or a + YAML seed hint); the ADB port is pre-filled with the standard 5555 so it + reads as an advanced setting rather than required knowledge (t33 F-D). """ defaults = defaults or {} + host = str(defaults.get("host") or "") + port = defaults.get("port") + if not isinstance(port, int): + port = DEFAULT_ADB_PORT schema_dict = { vol.Required("name", default=defaults.get("name", DEFAULT_NAME)): str, vol.Optional(CONF_ANDROIDTV_ENTITY): sel["androidtv"], @@ -174,14 +193,13 @@ def _user_schema_dict(sel, defaults: dict | None = None) -> dict: schema_dict[vol.Optional(CONF_POWER_ENTITY)] = sel["power"] schema_dict[vol.Optional(CONF_ADB_ENABLED)] = bool - schema_dict[vol.Optional(CONF_ADB_STORE_APP_INFO)] = bool - schema_dict[vol.Optional(CONF_ADB_HOST)] = str - schema_dict[vol.Optional(CONF_ADB_PORT)] = int + # the code's own fallback for this flag is True (async_step_user) — the + # rendered checkbox must agree with it + schema_dict[vol.Optional(CONF_ADB_STORE_APP_INFO, default=True)] = bool + schema_dict[_host_field_key(CONF_ADB_HOST, host)] = str + schema_dict[vol.Optional(CONF_ADB_PORT, default=port)] = int schema_dict[vol.Optional(CONF_PROTOCOL_ENABLED)] = bool - schema_dict[vol.Optional(CONF_PROTOCOL_HOST)] = str - if "host" in defaults: - schema_dict[vol.Optional(CONF_ADB_HOST)] = str - schema_dict[vol.Optional(CONF_PROTOCOL_HOST)] = str + schema_dict[_host_field_key(CONF_PROTOCOL_HOST, host)] = str return schema_dict @@ -215,6 +233,9 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): # Zeroconf discovery state (P4-1) self._discovered_host: str | None = None self._discovered_name: str | None = None + # YAML-seed prefill hint (t33 F-A): feeds the same user-step prefill as + # a discovery, without claiming the address was discovered. + self._seed_host: str | None = None async def async_step_user(self, user_input=None): """Step 1: Basic setup. @@ -236,12 +257,7 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): ) if not has_source: errors["base"] = "no_state_source" - sel = _build_selectors() - return self.async_show_form( - step_id="user", - data_schema=vol.Schema(_user_schema_dict(sel)), - errors=errors, - ) + return self._show_user_form(errors) # t29 G2: the androidtv entity_id is a renameable reference — # never a unique_id. A sqlite/registry MAC is preferred (the @@ -301,21 +317,48 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): return self._create_entry() + return self._show_user_form(errors) - sel = _build_selectors() + def _user_defaults(self) -> dict: + """Defaults for the user step: whatever setup already knows. - defaults = {} + A zeroconf discovery (or a YAML seed hint) supplies the address; the + port keeps a seed's hint instead of being flattened to the standard + value. This is the ONLY place the known address enters the form — the + schema keys carry these values as their defaults (t33 F-A). + """ + defaults = { + "name": self._discovered_name + or self._user_data.get("name") + or DEFAULT_NAME, + } + host = self._discovered_host or self._seed_host + if host: + defaults["host"] = host + port = self._user_data.get(CONF_ADB_PORT) + if isinstance(port, int): + defaults["port"] = port + return defaults + + def _show_user_form(self, errors=None): + """Render the user step with the known address actually applied. + + t33 F-A2: a discovery-driven form says so in its description, so a + pre-filled host can never be misread as "unknown". + """ + note = "" if self._discovered_host: - defaults = { - "name": self._discovered_name or DEFAULT_NAME, - "host": self._discovered_host, - } - schema_dict = _user_schema_dict(sel, defaults) - + note = ( + f"Discovered Shield at {self._discovered_host} — its address " + "is already filled in below. " + ) return self.async_show_form( step_id="user", - data_schema=vol.Schema(schema_dict), - errors=errors, + data_schema=vol.Schema( + _user_schema_dict(_build_selectors(), self._user_defaults()) + ), + errors=errors or {}, + description_placeholders={"discovery_note": note}, ) async def async_step_zeroconf(self, discovery_info): @@ -530,7 +573,10 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): # protocol prefill hints ONLY (pairing stays interactive) CONF_PROTOCOL_HOST: block.get(CONF_PROTOCOL_HOST, ""), } - self._discovered_host = self._user_data[CONF_PROTOCOL_HOST] or self._user_data[ + # t33 F-A: the seed host feeds the same user-step prefill as a + # zeroconf discovery, but WITHOUT the discovery announcement — the + # address came from configuration.yaml, not from the network. + self._seed_host = self._user_data[CONF_PROTOCOL_HOST] or self._user_data[ CONF_ADB_HOST ] return await self.async_step_user(None) diff --git a/custom_components/nvidia_shield/strings.json b/custom_components/nvidia_shield/strings.json index 96bffed..a9aa451 100644 --- a/custom_components/nvidia_shield/strings.json +++ b/custom_components/nvidia_shield/strings.json @@ -3,7 +3,7 @@ "step": { "user": { "title": "NVIDIA Shield", - "description": "Set up your NVIDIA Shield. Link the Android TV remote, Jellyfin, receiver, and power monitoring.", + "description": "{discovery_note}Set up your NVIDIA Shield. Link the Android TV remote, Jellyfin, receiver, and power monitoring.", "data": { "name": "Device Name", "androidtv_entity": "The media_player entity from the Android TV Remote integration. Optional for standalone setups that pair with the Nvidia Remote Protocol instead.", diff --git a/custom_components/nvidia_shield/translations/en.json b/custom_components/nvidia_shield/translations/en.json index 96bffed..a9aa451 100644 --- a/custom_components/nvidia_shield/translations/en.json +++ b/custom_components/nvidia_shield/translations/en.json @@ -3,7 +3,7 @@ "step": { "user": { "title": "NVIDIA Shield", - "description": "Set up your NVIDIA Shield. Link the Android TV remote, Jellyfin, receiver, and power monitoring.", + "description": "{discovery_note}Set up your NVIDIA Shield. Link the Android TV remote, Jellyfin, receiver, and power monitoring.", "data": { "name": "Device Name", "androidtv_entity": "The media_player entity from the Android TV Remote integration. Optional for standalone setups that pair with the Nvidia Remote Protocol instead.", diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index 19d6c73..d2d6a16 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -11,6 +11,8 @@ import enum import sys import types +import voluptuous_mirror + ENTITIES_STUB_NAMES = ( "voluptuous", "homeassistant", @@ -115,9 +117,9 @@ def install() -> None: return vol = types.ModuleType("voluptuous") - vol.Required = lambda key, default=None: key - vol.Optional = lambda key, default=None: key - vol.Schema = lambda schema: schema + # t33 F-A: the faithful mirror keeps schema defaults visible — a bare-key + # stub cannot tell a schema WITH a default from one without. + voluptuous_mirror.install(vol) sys.modules["voluptuous"] = vol ha = types.ModuleType("homeassistant") @@ -148,12 +150,16 @@ def install() -> None: # t32 F2 (audited helper): recording no-op at hub scope pass - def async_show_form(self, *, step_id, data_schema=None, errors=None): + def async_show_form( + self, *, step_id, data_schema=None, errors=None, + description_placeholders=None, + ): return { "type": "form", "step_id": step_id, "errors": dict(errors or {}), "data_schema": data_schema, + "description_placeholders": dict(description_placeholders or {}), } def async_create_entry(self, *, title, data, options=None): diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index bb7c0c0..1ecbd9e 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -15,6 +15,7 @@ from types import SimpleNamespace from unittest import mock import pytest +import voluptuous_mirror as vol_mirror from conftest import _SHIELD_DIR, _load_module from nsh.const import ( CONF_ADB_ENABLED, @@ -54,9 +55,10 @@ def _install_stubs() -> None: return vol = types.ModuleType("voluptuous") - vol.Required = lambda key, default=None: key - vol.Optional = lambda key, default=None: key - vol.Schema = lambda schema: schema + # t33 F-A: a faithful mirror — the default stays ON the key, so a schema + # whose host default was dropped FAILS here instead of looking identical + # to one that carries it (the old bare-key stub hid exactly that bug). + vol_mirror.install(vol) sys.modules["voluptuous"] = vol ha = types.ModuleType("homeassistant") @@ -71,12 +73,16 @@ def _install_stubs() -> None: kwargs.pop("domain", None) super().__init_subclass__(**kwargs) - def async_show_form(self, *, step_id, data_schema=None, errors=None): + def async_show_form( + self, *, step_id, data_schema=None, errors=None, + description_placeholders=None, + ): return { "type": "form", "step_id": step_id, "errors": dict(errors or {}), "data_schema": data_schema, + "description_placeholders": dict(description_placeholders or {}), } def async_create_entry(self, *, title, data, options=None): @@ -942,6 +948,11 @@ class TestZeroconfDiscovery: ) async def test_discovery_prefills_the_user_step(self, tmp_path): + """t33 F-A: the discovered host is the DEFAULT OF THE SCHEMA KEYS. + + The old check (the attribute was stored) stayed green while the + rendered form was blank — this one reads the schema itself. + """ module = _form_module() flow = module.NvidiaShieldConfigFlow() flow.hass = FakeHass(str(tmp_path)) @@ -950,9 +961,13 @@ class TestZeroconfDiscovery: assert result["type"] == "form" assert result["step_id"] == "user" - # pre-fill landed: the discovered host is recorded for the schema assert flow._discovered_host == "192.168.1.50" - assert result["data_schema"] is not None + defaults = vol_mirror.defaults_by_key(result["data_schema"]) + assert defaults[CONF_ADB_HOST] == "192.168.1.50" + assert defaults[CONF_PROTOCOL_HOST] == "192.168.1.50" + # ...and a discovery-driven form says the address is already known + note = result["description_placeholders"]["discovery_note"] + assert "Discovered Shield at 192.168.1.50" in note async def test_discovery_without_address_aborts(self, tmp_path): module = _form_module() diff --git a/tests/voluptuous_mirror.py b/tests/voluptuous_mirror.py new file mode 100644 index 0000000..362cc9b --- /dev/null +++ b/tests/voluptuous_mirror.py @@ -0,0 +1,89 @@ +"""A voluptuous mirror faithful enough to SEE a dropped schema default. + +The test environment never installs voluptuous (the config-flow tests import +``config_flow`` against import-only stubs), so the harness has to model the two +behaviours the flow schemas depend on: + +* ``Optional(field, default=value)`` / ``Required(field, default=value)`` carry + that default on the KEY object; +* keys compare by FIELD NAME only — real ``voluptuous.Marker`` implements + ``__hash__ = hash(self.schema)`` and ``__eq__`` on class + schema. A later + re-declaration of the same field therefore keeps the FIRST key object, so a + default added by a second declaration is silently dropped. + +That second property is exactly how the t33 F-A discovery-prefill bug hid from +the suite: the old stubs reduced every key to its bare string, so a schema +whose host key carried no default looked identical to one that carried the +discovered address. ``TestVoluptuousMirrorContract`` in +``tests/test_config_flow_ux.py`` pins both properties so the harness itself +cannot regress into blindness. +""" + +from __future__ import annotations + +UNDEFINED = object() +"""Sentinel for "this key declares no default" (real ``voluptuous.UNDEFINED``).""" + + +class Marker: + """Stand-in for ``voluptuous.Marker`` (the ``Required``/``Optional`` base).""" + + def __init__(self, schema, default=UNDEFINED): + self.schema = schema + self.default = default + + def __hash__(self): + # real Marker hashes the field name only + return hash(self.schema) + + def __eq__(self, other): + # real Marker compares class + field name, never the default + return type(other) is type(self) and other.schema == self.schema + + def __repr__(self): + return f"<{type(self).__name__} {self.schema!r} default={self.default!r}>" + + +class Required(Marker): + """Stand-in for ``voluptuous.Required``.""" + + +class Optional(Marker): + """Stand-in for ``voluptuous.Optional``.""" + + +def install(vol) -> None: + """Wire the mirror onto a stub ``voluptuous`` module object.""" + vol.UNDEFINED = UNDEFINED + vol.Marker = Marker + vol.Required = Required + vol.Optional = Optional + vol.Schema = lambda schema: schema + + +def defaults_by_key(schema_dict) -> dict: + """``{field: default}`` for a schema declared with mirror keys.""" + return {key.schema: key.default for key in declared_keys(schema_dict)} + + +def declared_keys(schema_dict) -> list: + """The schema's marker keys, in declaration order. + + Raises on a non-marker key: a bare string key means the harness (or the + code under test) lost the KEY structure this mirror exists to check. + Test-harness bookkeeping keys (``__suggested__`` and friends) are skipped. + """ + keys = [] + for key in schema_dict: + if isinstance(key, Marker): + keys.append(key) + continue + if isinstance(key, str) and key.startswith("__"): + continue + raise AssertionError(f"schema key {key!r} is not a voluptuous marker") + return keys + + +def declared_fields(schema_dict) -> list: + """The field names of a schema, in declaration order.""" + return [key.schema for key in declared_keys(schema_dict)]