From e11dccf2c8cd013a8eadd560a62f6c9f7dc0bce0 Mon Sep 17 00:00:00 2001 From: James Mackay Date: Sat, 10 Oct 2026 21:04:47 +0100 Subject: [PATCH] =?UTF-8?q?fix(config=5Fflow):=20F1=20=E2=80=94=20a=20disc?= =?UTF-8?q?overable=20later=20pairing=20path=20(Reconfigure)=20and=20truth?= =?UTF-8?q?ful=20skip=20copy=20(t35=20F1/F1b)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Continue without protocol pairing" switched the transport off for good: the options flow carries no protocol fields, so nothing in the UI could ever turn it back on, and the skip copy promised nothing about it (t34 F1). The entry now answers Home Assistant's Reconfigure action: * async_step_reconfigure (audit G5) seeds the flow from the entry (title, adb host/port/enabled, androidtv entity for the host fallback) and re-runs the pairing step — the address is editable, the Shield shows a PIN, and _finish_reconfigure persists protocol_enabled=True plus the address that just paired (so the transport no longer depends on the ADB fallback), adopts the paired MAC when it differs, and updates + reloads the entry. HA derives the "reconfigure_successful" abort from the flow source. A Reconfigure visit that clicks skip aborts honestly with "reconfigure_skipped" (entry untouched), and an entry that is gone aborts with "reconfigure_entry_missing". * the skip data_description now states the truth — the transport stays off until you pair it — and names the later path (the integration's entry menu > Reconfigure). * a host corrected on the pairing form now DROPS the session started against the old address and restarts pairing there, so the typed address is authoritative instead of being paired against the stale one. * the success path no longer runs _abort_if_unique_id_configured for a flow that already owns an entry: core looks the domain up by unique_id, so re-pairing an entry aborted on itself as "already_configured" (this also unblocked reauth). The fresh-setup path is unchanged. * adjacent fix while wiring the same call: _finish_reauth passed `options_updates`, which core's async_update_reload_and_abort does not accept (the kwarg is `options`, REPLACE) — the reauth path would have raised TypeError in production. Both paths now pass `options` and the test stubs mirror the real signature. Tests: tests/test_config_flow_ux.py::TestProtocolReconfigure drives reconfigure from a protocol-disabled entry through to updated options (transport on, host persisted, other options kept, MAC adopted, reload recorded), a corrected host reopening the PIN step against the new address, the skip abort, and the missing-entry abort; TestCopyContract gained the F1b copy pin. Against HEAD's config_flow.py all four reconfigure tests fail (no async_step_reconfigure). Verified: uvx --with pytest-asyncio pytest tests -q → 281 passed, 1 skipped; uvx --with pytest-asyncio --with androidtvremote2 pytest tests -q → 282 passed; uvx ruff check custom_components tests → clean. --- .../nvidia_shield/config_flow.py | 76 ++++++++- custom_components/nvidia_shield/strings.json | 4 +- .../nvidia_shield/translations/en.json | 4 +- tests/entity_stublib.py | 5 + tests/test_config_flow_protocol.py | 16 +- tests/test_config_flow_ux.py | 160 +++++++++++++++++- 6 files changed, 255 insertions(+), 10 deletions(-) diff --git a/custom_components/nvidia_shield/config_flow.py b/custom_components/nvidia_shield/config_flow.py index fdaec45..2486f4b 100644 --- a/custom_components/nvidia_shield/config_flow.py +++ b/custom_components/nvidia_shield/config_flow.py @@ -231,6 +231,8 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): self._user_data = {} self._protocol_client = None self._reauth_entry = None + # audit G5 / t35 F1: the entry a "Reconfigure" visit targets + self._reconfigure_entry = None # Zeroconf discovery state (P4-1) self._discovered_host: str | None = None self._discovered_name: str | None = None @@ -542,6 +544,11 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): # say so instead of pretending the re-pair worked self._reauth_entry = None return self.async_abort(reason="reauth_skipped") + if self._reconfigure_entry is not None: + # same for a Reconfigure visit: nothing changed, the entry keeps + # its options and can be reconfigured again at any time + self._reconfigure_entry = None + return self.async_abort(reason="reconfigure_skipped") return self._create_entry() async def _async_restart_protocol_pairing(self, errors: dict): @@ -578,9 +585,11 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): typed_host = str(user_input.get(CONF_PROTOCOL_HOST) or "").strip() if typed_host and typed_host != (self._protocol_host() or ""): - # A host typed HERE is authoritative — the user step is gone, and - # the session started against the old address is void. + # A host typed HERE is authoritative — the user step is gone, so + # the session started against the old address is dropped and the + # Shield is asked for a NEW code at the corrected address (t35 F1). self._user_data[CONF_PROTOCOL_HOST] = typed_host + self._discard_protocol_client() code = str(user_input.get(CONF_PAIRING_CODE, "") or "").strip() if self._protocol_client is None or not code: @@ -602,11 +611,19 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): with contextlib.suppress(Exception): client.disconnect() self._protocol_client = None - if mac: + target = self._reauth_entry or self._reconfigure_entry + if mac and mac != getattr(target, "unique_id", None): + # A MAC that is already configured must never be adopted silently. + # A reauth/reconfigure flow already OWNS an entry, and + # _abort_if_unique_id_configured looks the domain up by unique_id — + # it would abort on that very entry when the MAC is unchanged + # (t35 F1: re-pairing an entry used to abort as "already_configured"). await self.async_set_unique_id(mac) self._abort_if_unique_id_configured() if self._reauth_entry is not None: return self._finish_reauth() + if self._reconfigure_entry is not None: + return self._finish_reconfigure(mac) return self._create_entry() async def async_step_import(self, data): @@ -687,17 +704,68 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): step_id="reauth_confirm", data_schema=vol.Schema({}) ) + async def async_step_reconfigure(self, user_input=None): + """Re-run protocol pairing for an existing entry (audit G5, t35 F1). + + Home Assistant offers this as the entry's "Reconfigure" action — the + discoverable way back for an entry whose protocol transport is off + (typically the "continue without protocol pairing" outcome): the + address is editable, the Shield shows a PIN, and the entry's options + are updated + reloaded. + """ + entry = self._reconfigure_entry or self._get_reconfigure_entry() + if entry is None: + # HA always links the entry; a missing one gets a plain abort + # instead of an exception + return self.async_abort(reason="reconfigure_entry_missing") + self._reconfigure_entry = entry + # Seed the flow from the entry so the pairing step offers the address + # the entry already knows (and _create_entry is never reached here). + self._user_data = { + "name": entry.title, + CONF_ANDROIDTV_ENTITY: entry.data.get(CONF_ANDROIDTV_ENTITY, ""), + CONF_ADB_ENABLED: entry.options.get(CONF_ADB_ENABLED, False), + CONF_ADB_HOST: entry.options.get(CONF_ADB_HOST, ""), + CONF_ADB_PORT: entry.options.get(CONF_ADB_PORT, DEFAULT_ADB_PORT), + CONF_PROTOCOL_ENABLED: True, + CONF_PROTOCOL_HOST: entry.options.get(CONF_PROTOCOL_HOST, ""), + } + return await self.async_step_protocol_pair() + + def _finish_reconfigure(self, mac: str | None): + """Reconfigure pairing done: update + reload + abort (G3a contract). + + The protocol host persists as the address that just paired, so the + entry no longer depends on the ADB-host fallback for its transport. + HA derives the abort reason ("reconfigure_successful") from the flow + source, so this needs no integration string. + """ + entry = self._reconfigure_entry or self._get_reconfigure_entry() + updates: dict = { + "options": { + **entry.options, + CONF_PROTOCOL_ENABLED: True, + CONF_PROTOCOL_HOST: self._protocol_host() or "", + }, + } + if mac: + # a swapped device keeps the entry's identity honest + updates["unique_id"] = mac + self._reconfigure_entry = None + return self.async_update_reload_and_abort(entry, **updates) + def _finish_reauth(self): """Reauth pairing done: update + reload + abort via the current API (G3a). The deprecated update-entry + schedule-reload boomerang listener is gone (2026.12 treats it as an error); the current config-entries contract is async_update_reload_and_abort with _get_reauth_entry. + Core's kwarg is `options` (REPLACE) — there is no `options_updates`. """ entry = self._reauth_entry or self._get_reauth_entry() self.async_update_reload_and_abort( entry, - options_updates={ + options={ **entry.options, CONF_PROTOCOL_ENABLED: True, CONF_PROTOCOL_HOST: self._user_data.get(CONF_PROTOCOL_HOST, ""), diff --git a/custom_components/nvidia_shield/strings.json b/custom_components/nvidia_shield/strings.json index 3c4fb49..4b17e55 100644 --- a/custom_components/nvidia_shield/strings.json +++ b/custom_components/nvidia_shield/strings.json @@ -57,7 +57,7 @@ "data_description": { "pairing_code": "The code the Shield displays while this step is open.", "protocol_host": "The Shield's IP address. Pre-filled when the Shield was discovered \u2014 change it if the Shield moved.", - "skip_pairing": "Finishes setup now without the remote-protocol transport. Power, availability and the app push then come from the other state sources only." + "skip_pairing": "Finishes setup with the remote protocol off \u2014 it stays off until you pair it. Pair later from this integration's entry menu: choose Reconfigure." } }, "reauth_confirm": { @@ -80,6 +80,8 @@ "reauth_successful": "Re-paired successfully.", "reauth_skipped": "Pairing skipped. The saved certificate is still not trusted, so the remote protocol stays unavailable until you pair again.", "reauth_entry_missing": "This entry no longer exists.", + "reconfigure_skipped": "Nothing changed \u2014 the remote protocol stays off until you pair it. Choose Reconfigure from this integration's entry menu when you are ready.", + "reconfigure_entry_missing": "This entry no longer exists, so there is nothing to reconfigure.", "zeroconf_no_host": "The discovery event carried no usable address.", "yaml_seed_only": {}, "yaml_seed": { diff --git a/custom_components/nvidia_shield/translations/en.json b/custom_components/nvidia_shield/translations/en.json index 3c4fb49..4b17e55 100644 --- a/custom_components/nvidia_shield/translations/en.json +++ b/custom_components/nvidia_shield/translations/en.json @@ -57,7 +57,7 @@ "data_description": { "pairing_code": "The code the Shield displays while this step is open.", "protocol_host": "The Shield's IP address. Pre-filled when the Shield was discovered \u2014 change it if the Shield moved.", - "skip_pairing": "Finishes setup now without the remote-protocol transport. Power, availability and the app push then come from the other state sources only." + "skip_pairing": "Finishes setup with the remote protocol off \u2014 it stays off until you pair it. Pair later from this integration's entry menu: choose Reconfigure." } }, "reauth_confirm": { @@ -80,6 +80,8 @@ "reauth_successful": "Re-paired successfully.", "reauth_skipped": "Pairing skipped. The saved certificate is still not trusted, so the remote protocol stays unavailable until you pair again.", "reauth_entry_missing": "This entry no longer exists.", + "reconfigure_skipped": "Nothing changed \u2014 the remote protocol stays off until you pair it. Choose Reconfigure from this integration's entry menu when you are ready.", + "reconfigure_entry_missing": "This entry no longer exists, so there is nothing to reconfigure.", "zeroconf_no_host": "The discovery event carried no usable address.", "yaml_seed_only": {}, "yaml_seed": { diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index d2d6a16..2c355f0 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -173,6 +173,11 @@ def install() -> None: return entry return None # the caller's cached entry is used + def _get_reconfigure_entry(self, entry=None): + if entry is not None: + return entry + return None # the caller's cached entry is used + # NOTE: the real HA signature is async_update_reload_and_abort(entry, ...) # which updates, schedules the reload and aborts; the stub records both. diff --git a/tests/test_config_flow_protocol.py b/tests/test_config_flow_protocol.py index 91bbe92..268c0f5 100644 --- a/tests/test_config_flow_protocol.py +++ b/tests/test_config_flow_protocol.py @@ -158,12 +158,24 @@ def _install_stubs() -> None: ConfigFlow._get_reauth_entry = lambda self, entry=None: entry def _update_reload_and_abort(self, entry, **kwargs): - updates = kwargs.get("options_updates") or {} + # `options` REPLACES the entry options (the real signature); the merge + # form is still accepted for older callers. + updates = dict(kwargs.get("options_updates") or {}) + updates.update(kwargs.get("options") or {}) options = entry.options for key, value in updates.items(): options[key] = value + if kwargs.get("unique_id"): + entry.unique_id = kwargs["unique_id"] self.hass.config_entries.async_schedule_reload(entry.entry_id) - return self.async_abort(reason="reauth_successful") + reason = kwargs.get("reason") + if reason is None: + reason = ( + "reconfigure_successful" + if getattr(self, "source", None) == "reconfigure" + else "reauth_successful" + ) + return self.async_abort(reason=reason) ConfigFlow.async_update_reload_and_abort = _update_reload_and_abort diff --git a/tests/test_config_flow_ux.py b/tests/test_config_flow_ux.py index 11e8c26..1c6876e 100644 --- a/tests/test_config_flow_ux.py +++ b/tests/test_config_flow_ux.py @@ -120,11 +120,42 @@ def _install_stubs() -> None: def _get_reauth_entry(self, entry=None): return entry + def _get_reconfigure_entry(self, entry=None): + """Real HA reads the entry from the flow context (SOURCE_RECONFIGURE).""" + if entry is not None: + return entry + return self.hass.config_entries.async_get_entry( + (getattr(self, "context", None) or {}).get("entry_id") + ) + + @property + def source(self): + """Real HA exposes the flow's source through the context.""" + return (getattr(self, "context", None) or {}).get("source") + def async_update_reload_and_abort(self, entry, **kwargs): + """Real-HA shape: update, schedule the reload, then abort. + + ``options`` REPLACES the entry's options (core has no + ``options_updates``); the merge form is still accepted so the older + reauth call and this one can share the stub. HA picks the abort + reason from the flow source unless one is passed. + """ + for key, value in (kwargs.get("options") or {}).items(): + entry.options[key] = value for key, value in (kwargs.get("options_updates") or {}).items(): entry.options[key] = value + if kwargs.get("unique_id"): + entry.unique_id = kwargs["unique_id"] self.hass.config_entries.async_schedule_reload(entry.entry_id) - return self.async_abort(reason="reauth_successful") + reason = kwargs.get("reason") + if reason is None: + reason = ( + "reconfigure_successful" + if self.source == "reconfigure" + else "reauth_successful" + ) + return self.async_abort(reason=reason) class OptionsFlow: def async_show_form( @@ -423,7 +454,6 @@ class TestVoluptuousMirrorContract: with pytest.raises(AssertionError): vol_mirror.declared_fields({CONF_ADB_HOST: str}) - def test_marker_equality_is_field_only_like_voluptuous(self): """t35 F2: real ``Marker.__eq__`` is ``self.schema == other``. @@ -874,6 +904,123 @@ class TestNoDeadEndPairing: assert entry.options[CONF_PROTOCOL_ENABLED] is True # untouched +class TestProtocolReconfigure: + """F1: an entry whose protocol transport is off can pair LATER (audit G5).""" + + def _entry(self, **overrides): + options = { + CONF_PROTOCOL_ENABLED: False, # the "continue without pairing" outcome + CONF_PROTOCOL_HOST: "", + CONF_ADB_ENABLED: True, + CONF_ADB_HOST: "10.0.0.9", + CONF_ADB_PORT: 5555, + } + options.update(overrides) + return SimpleNamespace( + entry_id="e1", title="mdarcy", data={}, options=options, + unique_id=None, + ) + + def _reconfigure_flow(self, module, tmp_path, entry=None): + flow, hass = _new_flow(module, tmp_path) + if entry is not None: + hass.config_entries.by_id["e1"] = entry + flow.context = {"entry_id": "e1", "source": "reconfigure"} + return flow, hass + + async def test_reconfigure_pairs_a_protocol_disabled_entry( + self, tmp_path, monkeypatch + ): + """Host editable → PIN step → options updated → entry reloaded.""" + module = _form_module() + client = _patch_protocol(monkeypatch, module) + entry = self._entry() + flow, hass = self._reconfigure_flow(module, tmp_path, entry) + + form = await flow.async_step_reconfigure() + + assert form["step_id"] == "protocol_pair" # the PIN step + assert client.pairing_started is True + fields = vol_mirror.declared_fields(form["data_schema"]) + assert CONF_PROTOCOL_HOST in fields # the address is editable here + assert CONF_PAIRING_CODE in fields + + result = await flow.async_step_protocol_pair( + {CONF_PAIRING_CODE: FAKE_PAIRING_CODE} + ) + + assert result["reason"] == "reconfigure_successful" + # the transport is ON and carries the address that just paired + assert entry.options[CONF_PROTOCOL_ENABLED] is True + assert entry.options[CONF_PROTOCOL_HOST] == "10.0.0.9" + # the rest of the entry's options survive the update + assert entry.options[CONF_ADB_ENABLED] is True + assert entry.options[CONF_ADB_HOST] == "10.0.0.9" + assert entry.unique_id == FAKE_MAC + assert hass.config_entries.reloaded_entry_ids == ["e1"] + assert client.finished_codes == [FAKE_PAIRING_CODE] + + async def test_reconfigure_host_correction_reopens_the_pin_step( + self, tmp_path, monkeypatch + ): + """The editable host wins: a corrected address starts a NEW session.""" + module = _form_module() + from nsh.connection import ShieldConnectionManager + + first = FakeProtocolClient() + _patch_protocol(monkeypatch, module, first) + entry = self._entry() + flow, _hass = self._reconfigure_flow(module, tmp_path, entry) + await flow.async_step_reconfigure() + assert first.pairing_started is True + + second = FakeProtocolClient() + monkeypatch.setattr( + ShieldConnectionManager, "build_protocol_client", lambda *a, **k: second + ) + form = await flow.async_step_protocol_pair( + {CONF_PROTOCOL_HOST: "10.0.0.77", CONF_PAIRING_CODE: "0000"} + ) + + assert form["step_id"] == "protocol_pair" # asks for the NEW code + assert first.disconnects == 1 # the stale session was dropped + assert second.pairing_started is True # pairing followed the new address + assert first.finished_codes == [] # the stale code was never used + + result = await flow.async_step_protocol_pair( + {CONF_PAIRING_CODE: FAKE_PAIRING_CODE} + ) + + assert result["reason"] == "reconfigure_successful" + assert entry.options[CONF_PROTOCOL_HOST] == "10.0.0.77" + + async def test_reconfigure_skip_leaves_the_entry_untouched( + self, tmp_path, monkeypatch + ): + module = _form_module() + _patch_protocol(monkeypatch, module) + entry = self._entry() + flow, hass = self._reconfigure_flow(module, tmp_path, entry) + await flow.async_step_reconfigure() + + result = await flow.async_step_protocol_pair({CONF_SKIP_PAIRING: True}) + + assert result == {"type": "abort", "reason": "reconfigure_skipped"} + assert entry.options[CONF_PROTOCOL_ENABLED] is False # unchanged + assert hass.config_entries.reloaded_entry_ids == [] + + async def test_reconfigure_without_a_linked_entry_aborts( + self, tmp_path, monkeypatch + ): + module = _form_module() + _patch_protocol(monkeypatch, module) + flow, _hass = self._reconfigure_flow(module, tmp_path, entry=None) + + result = await flow.async_step_reconfigure() + + assert result == {"type": "abort", "reason": "reconfigure_entry_missing"} + + class TestCopyContract: """F-C: plain, self-consistent copy with no unreachable-step pointers.""" @@ -889,6 +1036,15 @@ class TestCopyContract: assert "host" in text, key assert "discovered" in text, key + def test_protocol_skip_copy_names_the_later_pairing_path(self): + """F1b: the transport stays OFF until paired, and the way back is named.""" + for file in (_strings(), _en_strings()): + text = file["config"]["step"]["protocol_pair"]["data_description"][ + "skip_pairing" + ] + assert "stays off until you pair it" in text + assert "Reconfigure" in text + def test_every_error_and_abort_key_the_flow_raises_exists_in_both_files(self): source = (BASE / "config_flow.py").read_text(encoding="utf-8") raised = set(re.findall(r'errors\["base"\] = "([a-z_]+)"', source))