feat(remote): the app/activity picker, and a protocol transport that really connects (t15)

The user's remote displayed but offered no app picker, and the protocol transport was never actually switched on. Both halves are fixed here. REMOTE PARITY. The entity declares RemoteEntityFeature.ACTIVITY — exactly what core's androidtv_remote declares — and publishes activity_list, which is what makes the frontend render an activity picker at all. The list is the app directory (the user's apps map, else the curated table as a floor) in map order, then the apps ADB discovery found and something can name, in the one name chain (user map > device label > curated > package id). Nothing nameless is listed as a bare package id, and current_activity is None until something is known: no activity is invented. remote.turn_on honours activity= by waking the device when it reports itself off and then launching through the existing launch path; a listed name resolves case-insensitively, a package id or a detail link passes through, and anything else raises unknown_activity naming the value instead of silently doing nothing. An empty/absent activity keeps the previous behaviour exactly, async_turn_off is untouched, and a configured command script still owns the sequence. WHY THE TRANSPORT WAS DEAD, MEASURED. androidtvremote2's keep_reconnecting only starts _async_reconnect, whose whole body is `while self._remote_message_protocol:` — false on the first check with nothing connected, so the task returned immediately: no connect attempt, no retry, and the auth callback was never reachable. HA core connects FIRST and only then keeps it alive. Our _start_protocol called keep_reconnecting after generating a certificate and never called async_connect anywhere, so every send_key_command/send_launch_app_command raised ConnectionClosed internally, the helpers swallowed it into `return False` plus a debug line, and no push (is_available/is_on/current_app/volume_info) ever arrived. The fix is a background connect loop: async_connect under a bounded timeout, network failures retried with the ADB watchdog's jitter and backoff, InvalidAuth routed into the EXISTING reauth callback (entry.async_start_reauth), and keep_reconnecting only after a session is up. Setup never blocks on a transport. A send that cannot run is no longer silent: it is counted, its error recorded verbatim, and logged at WARNING with the transport state, and the remote publishes protocol_state / protocol_connected / protocol_ever_authenticated / protocol_last_error / protocol_failures / protocol_certificate plus sticky per-push *_seen flags — so "sent" and "never switched on" can never look alike again. CASE (b) IS FIXED, NOT DOCUMENTED: switching the transport on in the options flow for an entry that never paired now routes into the real async_step_protocol_pair, and "continue without pairing" persists the transport OFF. One marker carries the pairing (CONF_PROTOCOL_PAIRED), written by the setup pairing step, the reauth finisher and the reconfigure finisher, so no entry can sit enabled-but-unpaired. The certificate from the first pairing is reused by design, so a paired entry is asked for no new PIN — which is also why the marker, not certificate material, is the source of truth. ADB STAYS THE PRIMARY STATE SOURCE. Arming the protocol introduces pushes that were never live before, so the reading now names its tier: manager.foreground_source drives the media player's app_source and the remote's activity_source, and the new tests prove a full protocol push set leaves the media state, the now-playing reading and every ADB store field untouched, while an ADB-only entry still serves media player, sensors, apps, scripts and receiver with only the protocol surface degraded. Copy: unknown_activity, activity_launch_failed and protocol_unavailable in BOTH strings.json and translations/en.json, pinned by a test; the README documents the picker, activity resolution, how to read the transport-state attributes, the script bypass (a script-routed press is never evidence of pairing), the connect-then-keep-alive lifecycle and cert reuse versus the other integration's own pairing. Evidence: 6 new real-HA FlowManager tests in tests/test_config_flow_real_ha.py (unpaired -> pairs, PIN -> marker, skip -> off, paired -> not asked again, and the setup flow's marker on success and on skip), a real-HA entity contract test in tests/test_ha_api_drift.py (exact supported_features equality with core, activity_list/current_activity reaching the state machine, live _attr_ updates) and new suites in tests/test_protocol_transport.py (connect-first, keep_reconnecting alone is a no-op, retry, auth rejection, honest send failures, ADB-primary precedence) and tests/test_command_routing.py (the activity contract, turn_on(activity=), script ownership, the transport attribute states). No device and no browser exist here: device behaviour is inferred from the library and core source, and every UI-facing claim is proven at the entity/registry/metadata level under real Home Assistant. Verify: suites 719/4 and 720/3; real-HA glob (tests/test_adb_command_real_ha.py + tests/test_config_flow_real_ha.py) 56 passed, 0 skipped; ruff clean; drift exit 0 (25 modules, 76 imports, 206 relative imports, 1322 statement blocks, 57 call arguments) with the floors raised 40->70, 100->195, 500->1300 and 52->55.
2026-10-12 12:15:58 +01:00
1 parent 6aa3b6bfdd
commit 14c168c25e
@@ -28,6 +28,7 @@ from .const import (
    CONF_PAIRING_CODE,
    CONF_POWER_ENTITY,
    CONF_PROTOCOL_ENABLED,
+    CONF_PROTOCOL_PAIRED,
    CONF_RECEIVER_ENTITY,
    CONF_RECEIVER_INPUT,
    CONF_REMOTE_COMMAND_SCRIPT,
@@ -338,6 +339,67 @@ def _build_selectors():
    }


+async def _async_start_protocol_pairing(
+    hass, host: str, client_name: str
+) -> tuple[object | None, str | None]:
+    """Build a throwaway pairing client and make the Shield show its PIN (t15).
+
+    Certificates live under ``.storage/<domain>/`` (core androidtv_remote helpers.py
+    pattern) and are SHARED between pairing and runtime — an entry paired later reuses
+    them, which is why enabling the transport on a paired entry asks for no new PIN.
+    Returns ``(client, None)`` or ``(None, error_key)``: no host, a missing
+    ``androidtvremote2`` and an unreachable Shield are all keys the form can show,
+    never an exception out of a step.
+
+    t44 F1: androidtvremote2 raises ``CannotConnect`` from both calls below, and the
+    protocol transport is ON by default, so an asleep, rebooting or mis-addressed
+    Shield is the ordinary case rather than an edge one. Left uncaught it escaped the
+    config flow entirely, and the protocol_pair form — the address field AND the
+    "continue without pairing" escape — never rendered.
+
+    Module-level (t15) because BOTH flows drive a pairing session: the setup flow on
+    its way in, and the OPTIONS flow when the transport is switched on for an entry
+    that never paired.
+    """
+    if not host:
+        return None, "protocol_no_host"
+    certfile, keyfile = await hass.async_add_executor_job(protocol_cert_paths, hass)
+    client = ShieldConnectionManager.build_protocol_client(
+        host,
+        client_name=client_name,
+        certfile=certfile,
+        keyfile=keyfile,
+    )
+    if client is None:
+        # build_protocol_client already logged the missing library; pairing cannot
+        # start, and the form still offers the skip path
+        return None, "protocol_unavailable"
+    try:
+        await client.async_generate_cert_if_missing()
+        await client.async_start_pairing()
+    except Exception as err:  # noqa: BLE001 - the library's error IS the finding
+        _LOGGER.warning(
+            "protocol pairing could not start at %s (%s: %s) — showing the "
+            "form so the address can be corrected or pairing skipped",
+            host,
+            type(err).__name__,
+            err,
+        )
+        _disconnect_pairing_client(client)
+        return None, "protocol_unreachable"
+    return client, None
+
+
+def _disconnect_pairing_client(client) -> None:
+    """Drop a THROWAWAY pairing client (t15) — both flows hold exactly one.
+
+    A pairing client never becomes the entry's runtime transport, so a session whose
+    address was corrected, or that was skipped, must be closed here rather than left
+    dangling with a live TLS connection.
+    """
+    if client is not None:
+        with contextlib.suppress(Exception):
+            client.disconnect()
 class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
    """Handle a config flow for Nvidia Shield."""

@@ -542,6 +604,10 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
                CONF_HOST: d.get(CONF_HOST, ""),
                CONF_ADB_PORT: d.get(CONF_ADB_PORT, DEFAULT_ADB_PORT),
                CONF_PROTOCOL_ENABLED: d.get(CONF_PROTOCOL_ENABLED, True),
+                # t15: the pairing marker travels with the entry: the pairing step set
+                # it (True) or the skip path cleared it (False), and the options flow
+                # reads it before it lets the transport run unpaired.
+                CONF_PROTOCOL_PAIRED: bool(d.get(CONF_PROTOCOL_PAIRED, False)),
            },
        )

@@ -563,57 +629,20 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
        """Drop a pairing client whose session became void (host corrected)."""
        client = self._protocol_client
        self._protocol_client = None
-        if client is not None:
-            with contextlib.suppress(Exception):
-                client.disconnect()
+        _disconnect_pairing_client(client)

    async def _start_protocol_pairing(self):
-        """Build a throwaway protocol client and make the Shield show its PIN.
+        """Build a throwaway protocol client and make the Shield show its PIN (t15).

-        Certificates live under ``.storage/<domain>/`` (core androidtv_remote
-        helpers.py pattern) and are shared between pairing and runtime. Returns
-        ``(client, None)`` or ``(None, error_key)`` — no host, a missing
-        ``androidtvremote2`` and an unreachable Shield are all keys the form can
-        show, never an exception out of the step.
-
-        t44 F1: androidtvremote2 raises ``CannotConnect`` from both calls below,
-        and the protocol transport is ON by default, so an asleep, rebooting or
-        mis-addressed Shield is the ordinary case rather than an edge one. Left
-        uncaught it escaped the config flow entirely, and the protocol_pair form
-        — the address field AND the "continue without pairing" escape — never
-        rendered.
+        Thin wrapper: the construction is module-level because the OPTIONS flow needs
+        exactly the same thing when the transport is switched on for an entry that
+        never paired (``_async_start_protocol_pairing``).
        """
-        host = self._host()
-        if not host:
-            return None, "protocol_no_host"
-        certfile, keyfile = await self.hass.async_add_executor_job(
-            protocol_cert_paths, self.hass
+        return await _async_start_protocol_pairing(
+            self.hass,
+            self._host(),
+            self._user_data.get("name", DEFAULT_NAME) or DEFAULT_NAME,
        )
-        client = ShieldConnectionManager.build_protocol_client(
-            host,
-            client_name=self._user_data.get("name", DEFAULT_NAME) or DEFAULT_NAME,
-            certfile=certfile,
-            keyfile=keyfile,
-        )
-        if client is None:
-            # build_protocol_client already logged the missing library;
-            # pairing cannot start, and the form still offers the skip path
-            return None, "protocol_unavailable"
-        try:
-            await client.async_generate_cert_if_missing()
-            await client.async_start_pairing()
-        except Exception as err:  # noqa: BLE001 - the library's error IS the finding
-            _LOGGER.warning(
-                "protocol pairing could not start at %s (%s: %s) — showing the "
-                "form so the address can be corrected or pairing skipped",
-                host,
-                type(err).__name__,
-                err,
-            )
-            with contextlib.suppress(Exception):
-                client.disconnect()
-            return None, "protocol_unreachable"
-        return client, None

    async def _async_begin_protocol_pairing(self):
        """Start a PIN pairing session: ``(client, None)`` or ``(None, error)``."""
@@ -647,6 +676,9 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
        """
        self._discard_protocol_client()
        self._user_data[CONF_PROTOCOL_ENABLED] = False
+        # t15: skipped means NOT paired — the marker is what keeps an unpaired client
+        # off instead of letting it look like a dead remote later.
+        self._user_data[CONF_PROTOCOL_PAIRED] = False
        if self._reauth_entry is not None:
            # an existing entry cannot be re-created: leave it untouched and
            # say so instead of pretending the re-pair worked
@@ -735,6 +767,12 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
        # t56: the protocol is paired now, so the setup menu must not offer to
        # pair it again — it returns to the menu (Receiver / Connection /
        # Finish) instead of creating the entry here.
+        #
+        # t15: and the pairing is PERSISTED. Nothing else can tell a paired entry
+        # from one that merely switched the transport on (the library has no
+        # is_paired, and certificate material is generated either way), so the
+        # options flow reads this marker before it lets the transport run unpaired.
+        self._user_data[CONF_PROTOCOL_PAIRED] = True
        self._paired_protocol = True
        return self._show_setup_menu()

@@ -851,6 +889,9 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
            "options": {
                **entry.options,
                CONF_PROTOCOL_ENABLED: True,
+                # t15: this reconfigure JUST paired — persist the marker the options
+                # flow reads before it lets an unpaired transport run.
+                CONF_PROTOCOL_PAIRED: True,
                CONF_HOST: self._host(),
            },
        }
@@ -874,6 +915,8 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
            options={
                **entry.options,
                CONF_PROTOCOL_ENABLED: True,
+                # t15: this reauth JUST paired — persist the marker (see above).
+                CONF_PROTOCOL_PAIRED: True,
                CONF_HOST: self._host(),
            },
        )
@@ -1194,6 +1237,9 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload):
        self._discover_note: str | None = None
        self._editing_package: str | None = None
        self._removing_package: str | None = None
+        # t15: the throwaway pairing client of a protocol_pair visit (see
+        # async_step_protocol_pair). Flow state, never an option.
+        self._protocol_client = None

    @property
    def _options_dict(self) -> dict:
@@ -1416,9 +1462,118 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload):
        """Main setup: the link, both transports and the linked entities."""
        errors: dict = {}
        if user_input is not None and self._apply_submission(dict(user_input), errors):
+            if self._protocol_enable_needs_pairing(user_input):
+                # t15: switching the transport ON for an entry that was never paired
+                # must PAIR now, not enable an unauthenticated client. The setup flow
+                # always paired (or turned the transport off); this is the way back in
+                # for an entry created in passive mode, and the only honest reading of
+                # "pair or stay off".
+                return await self.async_step_protocol_pair()
            return await self.async_step_menu_again()
        return self._show_connection_form(errors)

+    def _protocol_paired(self) -> bool:
+        """True when this entry's protocol client was really paired (t15).
+
+        The persisted marker is the ONLY honest source: the library exposes no
+        ``is_paired``, and certificate material proves only that something generated a
+        key — which an entry that merely enabled the transport has too (the runtime
+        calls ``async_generate_cert_if_missing``).
+        """
+        data = getattr(self.config_entry, "data", None) or {}
+        return bool(self._options_dict.get(CONF_PROTOCOL_PAIRED) or data.get(CONF_PROTOCOL_PAIRED))
+
+    def _protocol_enable_needs_pairing(self, submission: dict) -> bool:
+        """Does this submission turn the transport ON without a pairing to back it?"""
+        if CONF_PROTOCOL_ENABLED not in submission:
+            return False
+        if not self._options_dict.get(CONF_PROTOCOL_ENABLED):
+            return False  # turned off (or left off): nothing runs, nothing to pair
+        return not self._protocol_paired()
+
+    async def async_step_protocol_pair(self, user_input=None):
+        """Pair the remote protocol from the OPTIONS flow (t15).
+
+        Reached only by switching the transport ON for an entry that never paired. The
+        alternative is the state the user was stuck in: a client that connects nowhere,
+        no pushes, every send swallowed — a remote that looks unpaired whether it is or
+        not. ``skip_pairing`` leaves the transport OFF (persisted), so the entry cannot
+        end up enabled-but-unpaired either way.
+        """
+        errors: dict = {}
+
+        if user_input is None:
+            return await self._async_restart_options_pairing(errors)
+
+        if user_input.get(CONF_SKIP_PAIRING):
+            return await self._async_skip_options_pairing()
+
+        typed_host = str(user_input.get(CONF_HOST) or "").strip()
+        if typed_host and typed_host != entry_host(self.config_entry):
+            # A host typed HERE is authoritative (same rule as the setup flow): the
+            # session started against the old address is dropped and the Shield is
+            # asked for a NEW code at the corrected address.
+            self._options_dict[CONF_HOST] = typed_host
+            self._discard_options_pairing_client()
+
+        code = str(user_input.get(CONF_PAIRING_CODE, "") or "").strip()
+        if self._protocol_client is None or not code:
+            return await self._async_restart_options_pairing(errors)
+
+        try:
+            await self._protocol_client.async_finish_pairing(code)
+        except Exception:
+            errors["base"] = "protocol_pair_failed"
+            return self._show_options_pair_form(errors)
+
+        self._discard_options_pairing_client()
+        # Paired: the transport may run, and the marker keeps every later visit from
+        # asking again (the certificate is reused by design — no new PIN).
+        self._options_dict[CONF_PROTOCOL_PAIRED] = True
+        return await self.async_step_menu_again()
+
+    def _show_options_pair_form(self, errors=None):
+        """protocol_pair form for the OPTIONS flow: PIN, address, skip escape."""
+        host = entry_host(self.config_entry)
+        return self.async_show_form(
+            step_id="protocol_pair",
+            data_schema=vol.Schema(
+                {
+                    vol.Required(CONF_PAIRING_CODE): str,
+                    _host_field_key(CONF_HOST, host): str,
+                    vol.Optional(CONF_SKIP_PAIRING, default=False): bool,
+                }
+            ),
+            errors=errors or {},
+        )
+
+    async def _async_restart_options_pairing(self, errors: dict):
+        """Start a fresh pairing session and render the form for its code (t15)."""
+        self._discard_options_pairing_client()
+        client, error = await _async_start_protocol_pairing(
+            self.hass,
+            entry_host(self.config_entry),
+            self.config_entry.title or DEFAULT_NAME,
+        )
+        if error:
+            errors["base"] = error
+        else:
+            self._protocol_client = client
+        return self._show_options_pair_form(errors)
+
+    async def _async_skip_options_pairing(self):
+        """Continue with the transport OFF (t15) — never enabled-but-unpaired."""
+        self._discard_options_pairing_client()
+        self._options_dict[CONF_PROTOCOL_ENABLED] = False
+        self._options_dict[CONF_PROTOCOL_PAIRED] = False
+        return await self.async_step_menu_again()
+
+    def _discard_options_pairing_client(self) -> None:
+        """Drop the options flow's throwaway pairing client (t15)."""
+        client = self._protocol_client
+        self._protocol_client = None
+        _disconnect_pairing_client(client)
+
    def _show_connection_form(self, errors=None):
        """Main setup: the same settings the first setup form carries.