diff --git a/custom_components/googlefindmy/AGENTS.md b/custom_components/googlefindmy/AGENTS.md index d62a179c1..dd495154e 100644 --- a/custom_components/googlefindmy/AGENTS.md +++ b/custom_components/googlefindmy/AGENTS.md @@ -101,6 +101,17 @@ When a location decrypt/FCM callback encounters `SpotApiEmptyResponseError`, sto re-raise it after the waiter resumes so the coordinator can translate it into `ConfigEntryAuthFailed`. This keeps invalid sessions flowing into Home Assistant's reauthentication UI instead of being swallowed in background threads. +Classify an auth rejection by the HTTP **status**, never by the exception type. `NovaAuthError` is raised for every +non-retryable 4xx (`nova_request.py` raises it for 400, 404, 405, 409, 422 as well; `HTTP_RETRY_ELIGIBLE` holds no 4xx +besides 408 and 429), so a type check reads "device not found" as "your sign-in expired". A 401 that survived the refresh +sequence arrives flagged `is_permanent=True`, which leaves 403 as the only plain credential rejection a handler sees. +`api._classify_nova_auth_error` is the shared predicate: permanent first, then 401/403, everything else is a server-side +rejection. Extent today, stated so it is not mistaken for coverage: the two sound handlers use it; the device-list handler +(`api.py`, `except NovaAuthError` before `NovaProtobufDecodeError`), the location handler, `coordinator/polling.py` and +`coordinator/locate.py` still key off the type, and each of those turns a 404 into `ConfigEntryAuthFailed` or a reauth +countdown. Narrowing them is a behaviour change that needs its own regression test, not a drive-by edit; do not assume a +green suite proves the rest of the tree already follows this rule. + ### Import deferral reminder Heavyweight runtime dependencies (for example, browser drivers such as `undetected_chromedriver`) must be imported lazily inside diff --git a/custom_components/googlefindmy/api.py b/custom_components/googlefindmy/api.py index 9625084bd..2034e0b1f 100644 --- a/custom_components/googlefindmy/api.py +++ b/custom_components/googlefindmy/api.py @@ -12,6 +12,12 @@ - **401/403 (auth failures)** raised by Nova helpers are mapped to `homeassistant.exceptions.ConfigEntryAuthFailed` so the *coordinator* can trigger HA’s re-auth UX and Repairs issue workflow. + Known gap, stated so it is not mistaken for the current behaviour: the device + list and location handlers still key off the `NovaAuthError` TYPE, which the + transport also raises for non-credential 4xx (400, 404, 405, 409, 422), so a + deleted device can currently reach the re-auth flow. The sound handlers were + narrowed to the status; see `_classify_nova_auth_error`. Extending that to the + other two is a behaviour change of its own and is tracked separately. - **gpsoauth/ADM failures** (e.g., "BadAuthentication", "Missing 'Token' in gpsoauth") are normalized to `ConfigEntryAuthFailed` as well, even if they bubble up as a `RuntimeError`/`ValueError` rather than a `NovaAuthError`. @@ -43,6 +49,8 @@ CONTRIBUTOR_MODE_HIGH_TRAFFIC, CONTRIBUTOR_MODE_IN_ALL_AREAS, DEFAULT_CONTRIBUTOR_MODE, + PlaySoundResult, + SoundDispatchOutcome, ) from .NovaApi import nova_request from .NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( @@ -103,24 +111,66 @@ def _short_err(e: Exception | str) -> str: return msg -def _cancel_key_after_failure( - err: NovaError, request_uuid: str -) -> tuple[bool, str | None]: - """Decide the Play-Sound cancel-key fate for a non-accepted command. +def _classify_nova_auth_error(err: NovaAuthError) -> SoundDispatchOutcome: + """Name who refused when the transport raised ``NovaAuthError``. + + The exception type is wider than its name. ``nova_request`` raises it for + EVERY non-retryable 4xx, not only for credential rejections -- the comment + above that raise names "403 Forbidden, 404 Not Found" itself, and + ``HTTP_RETRY_ELIGIBLE`` holds no 4xx besides 408 and 429, so 400, 404, 405, + 409 and 422 all arrive here. A 401 that survived the refresh sequence is + raised separately with ``is_permanent=True``, which leaves 403 as the only + plain credential rejection reaching this handler. + + Reading the type alone therefore told a user with a deleted device to check + their sign-in. ``REJECTED_AUTH`` is for a refusal "on credentials: HTTP 401 + or 403", so a missing device or a malformed request belongs in + ``REJECTED_SERVER``, which names the non-credential client rejections + alongside the 5xx. Both docstrings state the criterion as the STATUS, not + the exception type; keep the three in step when any of them moves. + + Permanence outranks the status: ``NovaAuthPermanentError`` exists to say + "re-authentication is definitively required", so it stays ``REJECTED_AUTH`` + even if it ever carries a status outside 401/403. + + An error without a readable status keeps the old classification. That case + is a test double or a future subclass, not an observed server answer, and + the conservative reading of a type named "auth" is auth. + """ + if getattr(err, "is_permanent", False): + return SoundDispatchOutcome.REJECTED_AUTH + status = getattr(err, "status", None) + if status is None or status in (401, 403): + return SoundDispatchOutcome.REJECTED_AUTH + return SoundDispatchOutcome.REJECTED_SERVER + + +def _play_result_after_failure( + err: NovaError, request_uuid: str, outcome: SoundDispatchOutcome +) -> PlaySoundResult: + """Build the Play Sound result for a non-accepted command. + + Two independent facts are carried, and they must not be confused. ``outcome`` + names WHO refused: the server, the network, or this integration. ``cancel_key`` + answers only "may the device be ringing", which the transport latches onto the + error (``NovaError.dispatched``) at its retry-loop choke point. Before this + type existed, the second fact was the only one that survived the boundary, and + the coordinator had to reconstruct the first from it, which is why a server + rejection and a dead network both ended up as a push transport problem. A raised ``NovaError`` is never an acceptance (the submitter returns a tuple - only on HTTP 200), so ``success`` is always ``False``. The cancel key is - preserved ONLY when the failure latched dispatch (``err.dispatched``): some - attempt in the retry sequence reached the wire, so the device may already be - ringing and a later Stop needs the key. Pure rejections (401/403/5xx/429 + only on HTTP 200). The cancel key is preserved ONLY when the failure latched + dispatch (``err.dispatched``): some attempt in the retry sequence reached the + wire, so the device may already be ringing and a later Stop needs the key. + Pure rejections (401/403/5xx/429 with no wire-reaching attempt) and provable pre-dispatch failures inherit ``dispatched is False`` and drop the key, so they can never overwrite a previous, possibly still-ringing play's valid cancel key. Centralizing the decision here keeps every non-acceptance exit on one rule instead of - re-deriving it per ``except`` handler. See ``NovaError.dispatched`` and - IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY. + re-deriving it per ``except`` handler. See ``NovaError.dispatched``, + IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY and IRR-CA-SOUND-FAILURE-CLASS. """ - return (False, request_uuid if err.dispatched else None) + return PlaySoundResult(outcome, cancel_key=request_uuid if err.dispatched else None) # Backward-compatible export for tests and legacy call sites. @@ -266,17 +316,20 @@ def stop_sound(self, device_id: str, request_uuid: str | None = None) -> bool: """Stop playing sound on the device (sync wrapper).""" ... - async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: + async def async_play_sound(self, device_id: str) -> PlaySoundResult: """Play a sound on the device (async). - Returns (success, request_uuid) tuple. + Returns the classification and the cancel key; see PlaySoundResult. """ ... async def async_stop_sound( self, device_id: str, request_uuid: str | None = None - ) -> bool: - """Stop playing sound on the device (async).""" + ) -> SoundDispatchOutcome: + """Stop playing sound on the device (async). + + Returns the classification; see SoundDispatchOutcome. + """ ... @@ -1550,11 +1603,15 @@ def can_play_sound(self, device_id: str) -> bool | None: def play_sound(self, device_id: str) -> bool: """Thin sync wrapper around async_play_sound for non-HA contexts. + The classification is deliberately dropped here: this entry point exists + for CLI use and has no consumer that could act on it. HA callers use + ``async_play_sound`` and read ``PlaySoundResult.outcome``. + Args: device_id: The canonical ID of the device. Returns: - True if the command was sent successfully, False otherwise. + True if Nova accepted the command (HTTP 200), False otherwise. """ result = self._run_sync_helper( lambda: self.async_play_sound(device_id), @@ -1562,19 +1619,22 @@ def play_sound(self, device_id: str) -> bool: "play_sound() called inside an active event loop; use async_play_sound()." ), context=f"play sound on {device_id}", - default=(False, None), + default=PlaySoundResult(SoundDispatchOutcome.INTERNAL_ERROR), ) - success, _request_uuid = cast(tuple[bool, str | None], result) - return success + return cast(PlaySoundResult, result).accepted def stop_sound(self, device_id: str, request_uuid: str | None = None) -> bool: """Thin sync wrapper around async_stop_sound for non-HA contexts. + The classification is deliberately dropped here, for the same reason as in + ``play_sound``: this entry point is for CLI use. HA callers use + ``async_stop_sound`` and read the ``SoundDispatchOutcome``. + Args: device_id: The canonical ID of the device. Returns: - True if the command was sent successfully, False otherwise. + True if Nova accepted the command (HTTP 200), False otherwise. """ result = self._run_sync_helper( lambda: self.async_stop_sound(device_id, request_uuid), @@ -1582,36 +1642,45 @@ def stop_sound(self, device_id: str, request_uuid: str | None = None) -> bool: "stop_sound() called inside an active event loop; use async_stop_sound()." ), context=f"stop sound on {device_id}", - default=False, + default=SoundDispatchOutcome.INTERNAL_ERROR, ) - return cast(bool, result) + return cast(SoundDispatchOutcome, result) is SoundDispatchOutcome.ACCEPTED # ---------- Play/Stop Sound (async; HA-first) ---------- - async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: + async def async_play_sound(self, device_id: str) -> PlaySoundResult: """Send a 'Play Sound' command to a device (async path for HA). Auth mapping note: - If an auth error occurs here, we log and return False (service call context), - since re-auth is primarily driven by the coordinator’s data update path. + A credential rejection here is logged and returned as REJECTED_AUTH + (service call context), since re-auth is primarily driven by the + coordinator’s data update path. "Credential rejection" means the + STATUS says so (401/403) or the error is flagged permanent, not + merely that the transport raised ``NovaAuthError`` -- that type also + carries 400, 404 and the other non-retryable 4xx, which return + REJECTED_SERVER. See ``_classify_nova_auth_error``. Args: device_id: The canonical ID of the device. Returns: - A tuple `(success, request_uuid)` where `success` indicates whether the - command was *accepted* (HTTP 200) and `request_uuid` captures the - client-generated cancel key. The UUID is generated locally *before* + A `PlaySoundResult` carrying two independent facts. `outcome` names who + refused (see `SoundDispatchOutcome`); only `TRANSPORT_FAILED` describes + a broken push transport, and only that value may make a caller arm a + cooldown. `cancel_key` captures the client-generated cancel key and + answers one question only: may the device be ringing. Never derive the + cause from the key -- that out-of-band inference is what this type + removes. The UUID is generated locally *before* dispatch. It is returned (non-None) in two cases where the device may be ringing and a later Stop needs the key: - 1. Acceptance — the server answered 200. `success` is True. + 1. Acceptance — the server answered 200, `outcome` is `ACCEPTED`. 2. Post-dispatch ambiguity — a network failure occurred at or after the request reached the wire (server disconnect, read timeout, payload error), so the play may have started even though no 200 - was read. `success` is False, but the key is preserved. + was read. `outcome` is not `ACCEPTED`, but the key is preserved. Every failure that provably never reached the wire — no FCM token, missing cache, username/payload/token resolution, connection setup (DNS/connect refused/connect timeout) — and every explicit server - rejection (401/403/4xx/5xx/429) returns `(False, None)` so it cannot + rejection (401/403/4xx/5xx/429) returns a null `cancel_key` so it cannot overwrite a still-valid cancel key of a previous, possibly still-ringing play. The sole exception is a rejection/HTTP-status exit whose retry sequence latched dispatch on an *earlier* wire-reaching attempt: there @@ -1623,8 +1692,8 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: is additionally bound to "the cancel key was our own and fresh", so a foreign key passed in by a service call never evicts our handle. - True means Nova accepted the submission (HTTP 200). It is NOT a - confirmation that the device received or executed the command: no + `ACCEPTED` means Nova accepted the submission (HTTP 200). This is + NOT a confirmation that the device received or executed the command: no ExecuteActionResponse schema exists and no FCM callback is registered for sound, so nothing on this path can observe the ring. See IRR-CA-NO-RING-CONFIRMATION in @@ -1633,8 +1702,11 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: # Pass cache explicitly for multi-account isolation token = self._get_fcm_token_for_action() if not token: - # PRE-dispatch: nothing was sent, so there is no cancel key to keep. - return (False, None) + # PRE-dispatch: no transport was used at all, so there is no cancel key + # to keep and neither the server nor the network may be blamed. + # NOT_SENT keeps the caller from arming a push cooldown for a missing + # local token. + return PlaySoundResult(SoundDispatchOutcome.NOT_SENT) # Generate the cancel key locally *before* dispatch so it is in scope on # every path. Acceptance is still derived structurally from the submitter's # return contract: it returns a result tuple exclusively on an HTTP 200 and @@ -1669,9 +1741,11 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: ) # Defensive: the submitter returns a tuple on every accepted (200) # command and re-raises otherwise, so None is not an expected - # outcome. Treat the unconfirmed result conservatively — drop the - # key rather than risk overwriting a previous play's valid one. - return (False, None) + # outcome. That is a broken contract on our side, not an outage: + # INTERNAL_ERROR, never TRANSPORT_FAILED. Treat the unconfirmed + # result conservatively — drop the key rather than risk overwriting + # a previous play's valid one. + return PlaySoundResult(SoundDispatchOutcome.INTERNAL_ERROR) response_hex, _response_uuid = result _LOGGER.info("Play Sound (async) submitted successfully for %s", device_id) @@ -1682,22 +1756,44 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: len(response_hex) // 2 if response_hex else 0, response_hex[:200] if response_hex else "(empty)", ) - return (True, request_uuid) + return PlaySoundResult( + SoundDispatchOutcome.ACCEPTED, cancel_key=request_uuid + ) except NovaAuthError as err: - _LOGGER.error( - "Authentication failed while playing sound on %s: %s", - device_id, - _short_err(err), - ) + # The server answered and refused: never a transport failure, so an + # expired sign-in is not hidden behind a self-clearing 90-second + # cooldown. WHICH refusal it was is decided by the status, not by + # the exception type; see _classify_nova_auth_error. + outcome = _classify_nova_auth_error(err) + if outcome is SoundDispatchOutcome.REJECTED_AUTH: + _LOGGER.error( + "Authentication failed while playing sound on %s: %s", + device_id, + _short_err(err), + ) + else: + _LOGGER.warning( + "Client error (HTTP %s) while playing sound on %s: %s", + getattr(err, "status", "unknown"), + device_id, + _short_err(err), + ) # A raised error is never an acceptance (the submitter returns a tuple # only on a 200). Keep the cancel key only if an earlier attempt - # latched dispatch (err.dispatched): a pure 401/403 rejection with no + # latched dispatch (err.dispatched): a pure rejection with no # wire-reaching attempt drops it, so it can never overwrite a # previous, possibly still-ringing play's valid key. - return _cancel_key_after_failure(err, request_uuid) + return _play_result_after_failure(err, request_uuid, outcome) except NovaHTTPError as err: + # Non-acceptance (401/403/5xx/other): drop the key UNLESS an earlier + # attempt latched dispatch — a prior attempt that reached the server + # (a post-send network failure, or a 5xx/429 status read) may already + # be ringing. err.dispatched carries that sticky sequence latch + # (stamped at the transport's retry-loop choke point). Either way the + # server answered, so the transport worked and the outcome names the + # server, never the network. if getattr(err, "status", None) in (401, 403): _LOGGER.error( "Authentication failed (HTTP %s) while playing sound on %s: %s", @@ -1705,19 +1801,18 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: device_id, _short_err(err), ) - else: - _LOGGER.warning( - "Server error (%s) while playing sound on %s: %s", - err.status, - device_id, - _short_err(err), + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_AUTH ) - # Non-acceptance (401/403/5xx/other): drop the key UNLESS an earlier - # attempt latched dispatch — a prior attempt that reached the server - # (a post-send network failure, or a 5xx/429 status read) may already - # be ringing. err.dispatched carries that sticky sequence latch - # (stamped at the transport's retry-loop choke point). - return _cancel_key_after_failure(err, request_uuid) + _LOGGER.warning( + "Server error (%s) while playing sound on %s: %s", + err.status, + device_id, + _short_err(err), + ) + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_SERVER + ) except NovaRateLimitError as err: _LOGGER.warning( @@ -1725,8 +1820,40 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: ) # Same rule as the other non-acceptances: a rate-limited final attempt # never rang, but if a *prior* attempt reached the wire the latch - # (err.dispatched) preserves the key. - return _cancel_key_after_failure(err, request_uuid) + # (err.dispatched) preserves the key. The server answered; only the + # pace was wrong, so this is neither an outage nor a credential + # problem and gets its own value. + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_RATE_LIMIT + ) + + except NovaProtobufDecodeError as err: + # A 200 whose body we could not decode. The transport delivered; the + # payload broke on our side of the contract, so it is our bug, not an + # outage. Caught BEFORE `except NovaError` on purpose: that handler + # would otherwise classify it as TRANSPORT_FAILED and arm a cooldown + # for a decoding defect. + _LOGGER.error( + "Undecodable Play Sound response for %s: %s", + device_id, + _short_err(err), + exc_info=True, + ) + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.INTERNAL_ERROR + ) + + except NovaLogicError as err: + # HTTP 200 with an error code in the payload: the server answered and + # refused (unknown device, permission denied at API level). Same + # ordering rationale as above -- `except NovaError` would blame the + # network for a decision the server made. + _LOGGER.warning( + "Play Sound refused by Nova for %s: %s", device_id, _short_err(err) + ) + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.REJECTED_SERVER + ) except NovaError as err: # A network failure wrapped by async_nova_request after its retries, @@ -1735,9 +1862,9 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: # any attempt may have been processed by the server (a post-send # network failure, or a 5xx/429 status read), the play may already be # ringing, so keep the cancel key; a provable pre-connect failure - # never rang, so drop it. Other - # subclasses (NovaLogicError, NovaProtobufDecodeError) inherit - # dispatched=False and keep the conservative drop. + # never rang, so drop it. The two subclasses that are NOT transport + # failures (NovaLogicError, NovaProtobufDecodeError) are caught above, + # so what reaches this handler really is the transport giving up. if err.dispatched: _LOGGER.warning( "Play Sound for %s failed after reaching the server (%s); " @@ -1751,7 +1878,10 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: device_id, _short_err(err), ) - return _cancel_key_after_failure(err, request_uuid) + # The one outcome that justifies arming a push cooldown. + return _play_result_after_failure( + err, request_uuid, SoundDispatchOutcome.TRANSPORT_FAILED + ) except ClientError as err: # Defensive: async_nova_request wraps aiohttp errors into NovaError @@ -1763,22 +1893,33 @@ async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: device_id, _short_err(err), ) - return (False, None) + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED) except Exception as err: + # A bug on our side, not an outage. It gets a traceback and its own + # class, because reporting it as a transport failure is what put the + # integration into a self-inflicted 90-second cooldown. _LOGGER.error( - "Failed to play sound (async) on %s: %s", device_id, _short_err(err) + "Failed to play sound (async) on %s: %s", + device_id, + _short_err(err), + exc_info=True, ) - return (False, None) + return PlaySoundResult(SoundDispatchOutcome.INTERNAL_ERROR) async def async_stop_sound( self, device_id: str, request_uuid: str | None = None - ) -> bool: + ) -> SoundDispatchOutcome: """Send a 'Stop Sound' command to a device (async path for HA). Auth mapping note: - If an auth error occurs here, we log and return False (service call context), - since re-auth is primarily driven by the coordinator’s data update path. + A credential rejection here is logged and returned as REJECTED_AUTH + (service call context), since re-auth is primarily driven by the + coordinator’s data update path. "Credential rejection" means the + STATUS says so (401/403) or the error is flagged permanent, not + merely that the transport raised ``NovaAuthError`` -- that type also + carries 400, 404 and the other non-retryable 4xx, which return + REJECTED_SERVER. See ``_classify_nova_auth_error``. Args: device_id: The canonical ID of the device. @@ -1790,9 +1931,13 @@ async def async_stop_sound( limitation; see StopSoundOutcome.UNCORRELATED. Returns: - True if the command was submitted successfully, False otherwise. + A `SoundDispatchOutcome` naming who refused, on the same contract as + `async_play_sound`. Only `TRANSPORT_FAILED` describes a broken push + transport and may make a caller arm a cooldown; a server rejection, a + rate limit, a missing local token and a bug of our own each keep their + own value. - True means Nova accepted the submission (HTTP 200). It is NOT a + `ACCEPTED` means Nova accepted the submission (HTTP 200). It is NOT a confirmation that the device received or executed the command, and in particular not that the ring stopped: no ExecuteActionResponse schema exists and no FCM callback is registered for sound, so @@ -1802,7 +1947,9 @@ async def async_stop_sound( # Pass cache explicitly for multi-account isolation token = self._get_fcm_token_for_action() if not token: - return False + # PRE-dispatch: no transport was used, so neither the server nor the + # network may be blamed for the missing local token. + return SoundDispatchOutcome.NOT_SENT # Idempotent guard, not a second normalisation policy: the coordinator # funnel already maps blank to None. This entry point is public and # documented "for non-HA contexts", so a blank string can still arrive @@ -1862,15 +2009,32 @@ async def async_stop_sound( "empty response from server (no error details available)", device_id, ) - return bool(submitted) + # The submitter returns a hex body on acceptance and re-raises + # otherwise, so an empty reply breaks its own contract: our bug, not + # an outage. + if submitted: + return SoundDispatchOutcome.ACCEPTED + return SoundDispatchOutcome.INTERNAL_ERROR except NovaAuthError as err: - _LOGGER.error( - "Authentication failed while stopping sound on %s: %s", - device_id, - _short_err(err), - ) - return False + # The server answered and refused. The transport worked, so no + # cooldown may be armed for this. Same rule as on the play path: + # the status names the refusal, not the exception type. + outcome = _classify_nova_auth_error(err) + if outcome is SoundDispatchOutcome.REJECTED_AUTH: + _LOGGER.error( + "Authentication failed while stopping sound on %s: %s", + device_id, + _short_err(err), + ) + else: + _LOGGER.warning( + "Client error (HTTP %s) while stopping sound on %s: %s", + getattr(err, "status", "unknown"), + device_id, + _short_err(err), + ) + return outcome except NovaHTTPError as err: if getattr(err, "status", None) in (401, 403): @@ -1880,20 +2044,51 @@ async def async_stop_sound( device_id, _short_err(err), ) - return False + return SoundDispatchOutcome.REJECTED_AUTH _LOGGER.warning( "Server error (%s) while stopping sound on %s: %s", err.status, device_id, _short_err(err), ) - return False + return SoundDispatchOutcome.REJECTED_SERVER except NovaRateLimitError as err: _LOGGER.warning( "Stop Sound rate-limited for %s: %s", device_id, _short_err(err) ) - return False + # The server answered; only the pace was wrong. + return SoundDispatchOutcome.REJECTED_RATE_LIMIT + + except NovaProtobufDecodeError as err: + # Caught before `except NovaError` for the same reason as on the play + # path: an undecodable body is our defect, not a dead network. + _LOGGER.error( + "Undecodable Stop Sound response for %s: %s", + device_id, + _short_err(err), + exc_info=True, + ) + return SoundDispatchOutcome.INTERNAL_ERROR + + except NovaLogicError as err: + _LOGGER.warning( + "Stop Sound refused by Nova for %s: %s", device_id, _short_err(err) + ) + return SoundDispatchOutcome.REJECTED_SERVER + + except NovaError as err: + # A network failure wrapped by async_nova_request after its retries, + # or any other NovaError leaving the transport. Before the sound + # contract existed this fell through to `except Exception` and was + # reported as a plain False, indistinguishable from a server saying + # no. It is the one outcome that justifies arming a push cooldown. + _LOGGER.error( + "Network error while stopping sound on %s: %s", + device_id, + _short_err(err), + ) + return SoundDispatchOutcome.TRANSPORT_FAILED except ClientError as err: _LOGGER.error( @@ -1901,13 +2096,17 @@ async def async_stop_sound( device_id, _short_err(err), ) - return False + return SoundDispatchOutcome.TRANSPORT_FAILED except Exception as err: + # Our own bug: traceback, own class, and never a cooldown. _LOGGER.error( - "Failed to stop sound (async) on %s: %s", device_id, _short_err(err) + "Failed to stop sound (async) on %s: %s", + device_id, + _short_err(err), + exc_info=True, ) - return False + return SoundDispatchOutcome.INTERNAL_ERROR if ( diff --git a/custom_components/googlefindmy/const.py b/custom_components/googlefindmy/const.py index 644f08fc8..ea6939abe 100644 --- a/custom_components/googlefindmy/const.py +++ b/custom_components/googlefindmy/const.py @@ -11,6 +11,7 @@ import math import time from collections.abc import Mapping, Sequence +from dataclasses import dataclass from enum import StrEnum from types import MappingProxyType from typing import Final, Literal @@ -35,6 +36,107 @@ # would silently skip this file on the automated version bump. INTEGRATION_VERSION = "1.7.15.10" +# -------------------------------------------------------------------------------------- +# Sound dispatch outcome (API -> coordinator boundary) +# -------------------------------------------------------------------------------------- + + +class SoundDispatchOutcome(StrEnum): + """Why a sound command did or did not reach Google, as classified by ``api.py``. + + ``api.py`` already reaches nine distinct exits (missing action token, empty + submitter reply, HTTP 200, NovaAuthError, HTTP 401/403, other HTTP status, + rate limit, network error, unexpected exception). Until this type existed, + all of them collapsed into a single ``False``, and the coordinator had no + choice but to read that ``False`` as "the push transport is broken". A server + saying no and a network that never answered then produced the same 90-second + cooldown and the same ``FcmStatus.DEGRADED``. This is the same failure class + ``StopSoundOutcome`` documents one layer up: a bool cannot carry the state + space. + + Only ``TRANSPORT_FAILED`` justifies a push cooldown. Only ``ACCEPTED`` proves + that the credentials worked. + """ + + ACCEPTED = "accepted" + """Nova answered HTTP 200. Acceptance of the submission, not proof of a ring. + + See IRR-CA-NO-RING-CONFIRMATION in ``docs/PLAY_SOUND_ARCHITECTURE.md``. + """ + + REJECTED_AUTH = "rejected_auth" + """The server answered and refused on credentials: HTTP 401 or 403. + + The transport worked. A cooldown would mislabel an expired sign-in as a + network outage and hide it behind a self-clearing timer. + + The criterion is the STATUS, not the exception type. ``NovaAuthError`` is + raised for every non-retryable 4xx (its own docstring says "4xx client + errors", and ``HTTP_RETRY_ELIGIBLE`` holds no 4xx besides 408 and 429), so + reading the type alone filed a deleted device under "check your sign-in". + ``NovaAuthPermanentError`` and any error flagged ``is_permanent`` belong + here whatever their status: they say re-authentication is required. + """ + + REJECTED_RATE_LIMIT = "rejected_rate_limit" + """The server answered HTTP 429. The transport worked; only the pace was wrong.""" + + REJECTED_SERVER = "rejected_server" + """The server answered and refused for any other reason. + + Covers 5xx, a Nova logic error, and the non-credential client rejections -- + 400, 404, 405, 409, 422 -- that arrive as ``NovaAuthError`` despite naming + no credential problem. The enum has no ``REJECTED_CLIENT`` member because + no consumer branches on the distinction; what the two share, and what the + name is chosen for, is that the SERVER answered, so the transport worked + and no cooldown may be armed. For an HTTP refusal the log line names the + status; a Nova logic error carries no HTTP status (the request was a 200) + and its log line names the payload error code instead. + """ + + TRANSPORT_FAILED = "transport_failed" + """No usable answer was obtained: DNS, connect refused/timeout, disconnect, + read timeout, or a NovaError leaving the transport after its retries. + + This is the ONLY outcome that justifies arming the push cooldown. + """ + + NOT_SENT = "not_sent" + """A local precondition failed before any transport was used (no action token). + + Neither the server nor the network said anything, so neither may be blamed. + """ + + INTERNAL_ERROR = "internal_error" + """This integration failed on its own: an unexpected exception, or a contract + violation such as an empty reply from the submitter. + + Classifying such a bug as a network outage is what this type was written to + stop; it is logged with a traceback and never arms the cooldown. + """ + + +@dataclass(frozen=True, slots=True) +class PlaySoundResult: + """Result of ``api.async_play_sound``: a classification plus the cancel key. + + ``cancel_key`` is non-None in exactly the cases where the device may be + ringing and a later Stop needs the handle: an accepted command, or a failure + that latched dispatch (``NovaError.dispatched``). Read ``cancel_key`` ONLY for + the question "may the device be ringing"; read ``outcome`` for every question + about the cause. Deriving the cause from the key is the out-of-band channel + this type replaces. + """ + + outcome: SoundDispatchOutcome + cancel_key: str | None = None + + @property + def accepted(self) -> bool: + """Return True when Nova accepted the submission (HTTP 200).""" + return self.outcome is SoundDispatchOutcome.ACCEPTED + + # -------------------------------------------------------------------------------------- # Stop Sound outcome # -------------------------------------------------------------------------------------- @@ -52,9 +154,14 @@ class StopSoundOutcome(StrEnum): The two failure states are kept apart for the same reason the middle state exists: they differ in what the user has to do about them. ``SUPPRESSED`` - never left this machine and clears itself; ``FAILED`` reached the transport - and may need credentials, a network, or patience with a rate limit. One - message cannot advise on both without misleading half its readers. + clears itself and is answered by waiting; ``FAILED`` may need credentials, a + network, or patience with a rate limit. One message cannot advise on both + without misleading half its readers. + + They are NOT told apart by how far the request travelled. Both contain a + member that never left the machine -- see ``FAILED`` on + ``SoundDispatchOutcome.NOT_SENT`` -- and reading "did it reach the wire" + as the criterion is what makes a missing action token look suppressible. Note that even ``CANCELLED`` means "submitted with a correlated cancel key", not "the device stopped". Nova returns no parsable ExecuteActionResponse, so @@ -84,16 +191,37 @@ class StopSoundOutcome(StrEnum): Today the sole cause is a push transport that is not ready yet. The command never reached the network, the condition is local and transient, and the only useful advice is to retry shortly. + + "Never sent" alone does not qualify: ``SoundDispatchOutcome.NOT_SENT`` (the + readiness gate let the command through, but no action token was to be had) + never leaves the machine either and is filed under ``FAILED``. The dividing + line is the remedy, not the distance travelled -- see ``FAILED``. """ FAILED = "failed" - """Handed to the transport, but not accepted. - - Covers every way the attempt can die once it leaves this integration: - missing action token, authentication failure, HTTP 401/403, server error, - rate limit, network error, and an empty reply. These need different remedies - from ``SUPPRESSED`` -- often re-authentication -- so telling the user to - "try again in a moment" would be wrong for most of them. + """The attempt was made, and it did not succeed. + + Covers every way it can die once this integration has committed to making + it: authentication failure, HTTP 401/403, a server rejection, a rate limit, + a network error, an empty reply -- and the missing action token, which + never reaches the wire. + + That last member is why this docstring does not read "handed to the + transport". ``SoundDispatchOutcome.NOT_SENT`` is filed here rather than + under ``SUPPRESSED`` because the two failure states are told apart by the + REMEDY they call for, not by how far the request travelled. ``SUPPRESSED`` + means "the push transport is not up yet, try again in a moment", and it is + reached only by the readiness gate saying so. ``NOT_SENT`` is what the gate + could NOT see: it inspects the receiver's state (``is_push_ready``) while + the token is fetched separately (``_get_fcm_token_for_action``), so a + command the gate waved through can still find no action token. That is a + setup or credential problem far more often than a moment's patience, and + the advice such a user needs is the one ``stop_sound_rejected`` already + gives -- "an expired sign-in, a missing token, a server or network error, + or a rate limit", plus a look at the log. Re-authentication is the usual + remedy across this member, which is why "try again in a moment" would be + wrong for most of it. Routing ``NOT_SENT`` to ``SUPPRESSED`` would answer a + missing token with an unbounded "try again shortly". """ diff --git a/custom_components/googlefindmy/coordinator/locate.py b/custom_components/googlefindmy/coordinator/locate.py index ad4332339..7b4f64733 100644 --- a/custom_components/googlefindmy/coordinator/locate.py +++ b/custom_components/googlefindmy/coordinator/locate.py @@ -25,7 +25,11 @@ from homeassistant.exceptions import ConfigEntryAuthFailed, HomeAssistantError from .._reauth_reason import ReauthReasonCode -from ..const import DEFAULT_MIN_POLL_INTERVAL, StopSoundOutcome +from ..const import ( + DEFAULT_MIN_POLL_INTERVAL, + SoundDispatchOutcome, + StopSoundOutcome, +) from ..NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( DecryptionError, OwnerKeyLookupTransientError, @@ -720,6 +724,78 @@ def _cached_sound_uuid_is_stale(self, device_id: str) -> bool: return False return is_sound_uuid_expired(existing_ts, time.time(), SOUND_UUID_MAX_AGE_S) + def _note_stop_transport_problem_without_extending(self) -> None: + """Report a failed stop as a transport problem without LENGTHENING a live window. + + ``_note_push_transport_problem()`` does two things: it flags the + transport ``DEGRADED``, and it sets ``_push_cooldown_until`` to + ``monotonic() + cooldown_s`` ABSOLUTELY. The second part means calling + it while a window is already running restarts that window instead of + topping it up. Only the restart is unwanted here, so the call is made + in full and the window is put back afterwards -- skipping the call + outright would also drop the status flag, and a transport that just + failed must not keep reporting itself healthy because a location push + happened to arrive earlier in the same window. + + Reaching this code with a live window is close to new. Before the + correlated-stop exception in the gate, ``async_stop_sound`` returned + SUPPRESSED as its first statement whenever ``_api_push_ready()`` said + no, and a running window is one of the reasons it says no; only the + interleaving below could get past that. + + What the restart would cost: the stop button has no availability guard + (there is no ``can_stop_sound`` on the coordinator, ``button.py`` only + probes for one), so a user pressing Stop during an outage would push the + end of the window forward with every press. The window would still be + bounded -- it never outlives ``last press + cooldown_s`` -- but it also + gates manual locate unconditionally (and Play Sound for every device + whose ``can_ring`` capability is not cached, see ``can_play_sound()``), + and those stay disabled for as long as the pressing goes on. + + The stop itself is still sent; only the window is left alone. The same + rule covers a rare interleaving that predates this package: if a + concurrent play armed a window while this stop was in flight, that + window stands rather than being restarted from here. + """ + now = time.monotonic() + window_ends_at = self._push_cooldown_until + self._note_push_transport_problem() + if now < window_ends_at: + self._push_cooldown_until = window_ends_at + _LOGGER.debug( + "Stop failed on the transport while a push cooldown was " + "already running; keeping the existing window instead of " + "restarting it" + ) + + def _stop_would_be_correlated( + self, device_id: str, request_uuid: str | None + ) -> bool: + """Return True if a stop for ``device_id`` would carry a PROVEN cancel key. + + Read-only, no side effects. ``request_uuid`` must already be normalised + (a blank string collapsed to ``None``); ``async_stop_sound`` does that + at its single normalisation point before anything calls this. + + Proven means exactly one thing: the key that would go on the wire is our + own cached key and it is still fresh. An explicitly passed foreign + string is a claim, not a handle, and an aged key of ours cannot vouch + for the ring that is audible now. Both of those end as + ``StopSoundOutcome.UNCORRELATED``. + + This exists as one predicate because two decisions depend on the same + answer and must never drift apart: whether the stop may break a + self-inflicted push cooldown (IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN), + and whether an accepted stop may spend -- pop -- the cached key + (IRR-CA-POP-ON-CORRELATED-CANCEL-ONLY). + """ + cached_uuid = self._sound_request_uuids.get(device_id) + if cached_uuid is None: + return False + if request_uuid is not None and request_uuid != cached_uuid: + return False + return not self._cached_sound_uuid_is_stale(device_id) + async def async_play_sound(self, device_id: str) -> bool: """Play sound on a device using the native async API (no executor). @@ -742,7 +818,14 @@ async def async_play_sound(self, device_id: str) -> bool: ) return False try: - ok, request_uuid = await self.api.async_play_sound(device_id) + play = await self.api.async_play_sound(device_id) + # api.async_play_sound carries two independent facts. ``accepted`` + # answers "did Nova take the command", ``cancel_key`` answers "may + # the device be ringing", and ``outcome`` names WHO refused. The + # cause is read from ``outcome`` below and never reconstructed from + # the presence of a key -- that out-of-band inference is what + # IRR-CA-SOUND-FAILURE-CLASS removed. + ok, request_uuid = play.accepted, play.cancel_key # Decide whether to (over)write the cached Stop cancel key. # api.async_play_sound returns a non-None UUID in exactly the two # cases where a ring may be active and Stop needs the key: (1) the @@ -784,11 +867,26 @@ async def async_play_sound(self, device_id: str) -> bool: "Stored Play Sound UUID for %s: %s", device_id, request_uuid ) await self._async_save_sound_uuids() - if not ok: + # Only a transport that gave us no usable answer is a push problem. + # A server rejection (401/403/5xx), a rate limit, a missing local + # action token and a bug of our own all reached this point as the + # same False before SoundDispatchOutcome existed, so every one of + # them armed the 90-second cooldown, flipped the integration to + # FcmStatus.DEGRADED and made can_play_sound report the button as + # unavailable -- an outage this integration inflicted on itself over + # a network that was working, and one that then also suppressed the + # user's follow-up Stop. See IRR-CA-SOUND-FAILURE-CLASS. + if play.outcome is SoundDispatchOutcome.TRANSPORT_FAILED: self._note_push_transport_problem() - # Success implies credentials worked - self._set_auth_state(failed=False) - return bool(ok) + elif ok: + # Only an ACCEPTED submission proves the credentials worked. + # An auth rejection arrives as REJECTED_AUTH, indistinguishable + # from a read timeout before this contract existed, so clearing + # the auth-failure state on a failed play erased the very signal + # an expired sign-in produces. async_stop_sound has always + # applied this rule and states the reason; the two paths agree. + self._set_auth_state(failed=False) + return ok except ConfigEntryAuthFailed as auth_exc: self._set_auth_state( failed=True, reason=f"Auth failed during play_sound: {auth_exc}" @@ -815,7 +913,12 @@ async def async_play_sound(self, device_id: str) -> bool: exc_info=True, ) self.note_error(err, where="async_play_sound", device=device_id) - self._note_push_transport_problem() + # No cooldown here. api.async_play_sound classifies every Exception + # in band and returns INTERNAL_ERROR instead of raising, so nothing + # that reaches this handler came from the push transport: what is + # left is this method's own body around the call, or an api + # implementation that breaks the Protocol. Blaming the transport for + # either is the misclassification IRR-CA-SOUND-FAILURE-CLASS stops. return False async def async_stop_sound( @@ -838,20 +941,14 @@ async def async_stop_sound( A :class:`StopSoundOutcome`. The state space is four-valued on purpose: ``CANCELLED`` (submitted with a correlated cancel key), ``UNCORRELATED`` (submitted without one, so nothing proves an - effect), ``SUPPRESSED`` (never sent by this integration) and - ``FAILED`` (handed to the transport and not accepted). A bool cannot - carry the middle state, and collapsing it into success is what - reported a stop for a ring that kept playing (BSkando#195). + effect), ``SUPPRESSED`` (declined here because the push transport is + not up yet, so waiting is the remedy) and ``FAILED`` (attempted and + unsuccessful, which includes the missing action token that never + reaches the wire). A bool cannot carry the middle state, and + collapsing it into success is what reported a stop for a ring that + kept playing (BSkando#195). The two failure states split by REMEDY, + not by distance travelled; see ``StopSoundOutcome``. """ - # Less strict than can_play_sound(): stopping is harmless but still requires push readiness. - if not self._api_push_ready(): - _LOGGER.debug( - "Suppressing stop_sound call for %s: push not ready", device_id - ) - # A suppressed stop is a stop that was never sent, so it is a - # failure and must reach the service layer as one -- but as its own - # kind: nothing left this machine, and the condition clears itself. - return StopSoundOutcome.SUPPRESSED # Blank means "no opinion" -- an optional field left empty, a template # that rendered to nothing -- so it must fall through to the cached key # below, never pose as one. In-process the absence of a key already has @@ -865,8 +962,73 @@ async def async_stop_sound( # True only when the key about to be sent came from our own cache AND # was still fresh. An explicitly passed foreign UUID does NOT qualify: # popping our cache entry on its behalf would drop the handle of a - # different, possibly still running ring. - used_own_fresh_key = False + # different, possibly still running ring. Decided here, ABOVE the + # readiness gate, because that gate needs the same answer; the single + # definition lives in _stop_would_be_correlated(). + used_own_fresh_key = self._stop_would_be_correlated( + device_id, request_uuid_to_use + ) + + # Less strict than can_play_sound(): stopping is harmless but still + # requires push readiness. + if not self._api_push_ready(): + # ... with exactly one exception, and it runs OPPOSITE to + # can_play_sound() and the manual-locate guard above, which treat an + # active cooldown as the hard block and a merely unconfirmed + # transport as passable. + # + # What the exception tests is narrower than "the transport is + # fine", and it has to be: while a cooldown runs, _api_push_ready() + # short-circuits on that cooldown and never asks the transport at + # all, so the transport state is simply unknown here. The two cases + # this guard can tell apart are "not ready BECAUSE a window is + # running" and "not ready for some other reason", and only the + # first is passable. Claiming it also excludes a genuinely dead + # transport would be untrue: a transport failure is the ONLY thing + # that arms this window, so the two overlap by construction. + # + # Why the first case is passable at all: a play that reached the + # wire and lost the answer stores a cancel key and arms the 90 s + # window in the same breath, so the stop that key exists for was + # locked out for the first 90 seconds after that play -- which is + # exactly when a user reaches for the Stop button + # (IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN). Nothing here claims + # to know how long the ring itself lasts; that is a different timer + # on a different layer, see the note on the aged cached key below. + # The window is global while keys are per device, so "the play that + # armed it" is the common case, not a proven one. What IS proven is + # that this stop can be correlated, and that is the entire benefit + # bought here. + # + # The exception is bound to a PROVEN key, so every case that could + # at best report UNCORRELATED stays suppressed. It is not free, + # though, and the price is a changed outcome CLASS: a stop that + # used to end as SUPPRESSED ("not sent, try again in a moment") now + # reaches the transport, so it can also end as FAILED, which + # services.py reports with a different message. That is the honest + # trade -- an attempt that can actually silence the device, at the + # cost of a report that names the transport instead of the gate -- + # and it is pinned by a test rather than left implicit. + # + # It also cannot feed itself: a broken-through stop that fails on + # the transport leaves the window exactly where it was, see + # _note_stop_transport_problem_without_extending(). + if used_own_fresh_key and time.monotonic() < self._push_cooldown_until: + _LOGGER.debug( + "Push cooldown active for %s, but this stop carries our own " + "fresh cancel key: sending it anyway", + device_id, + ) + else: + _LOGGER.debug( + "Suppressing stop_sound call for %s: push not ready", device_id + ) + # A suppressed stop is a stop that was never sent, so it is a + # failure and must reach the service layer as one -- but as its + # own kind: nothing left this machine, and the condition clears + # itself. + return StopSoundOutcome.SUPPRESSED + cached_uuid_was_expired = False if request_uuid_to_use is None: cached_uuid = self._sound_request_uuids.get(device_id) @@ -910,7 +1072,6 @@ async def async_stop_sound( device_id, ) else: - used_own_fresh_key = True _LOGGER.debug( "Using cached Play Sound UUID for %s: %s", device_id, @@ -922,14 +1083,19 @@ async def async_stop_sound( # cached key. Any other string -- a typo, a stale template, the key # of a different ring -- is unverifiable, and reporting CANCELLED # for it would be BSkando#195 one layer up: success without effect. + # Which of the two it is was already settled by + # _stop_would_be_correlated() above; re-deriving the predicate here + # is exactly the drift this package removed. What is left for this + # branch is to say, per case, what it means. cached_uuid = self._sound_request_uuids.get(device_id) - if ( - cached_uuid is not None - and cached_uuid == request_uuid_to_use - and not self._cached_sound_uuid_is_stale(device_id) - ): - # It is our key, so spending it is correct, not an eviction. - used_own_fresh_key = True + if used_own_fresh_key: + # It is our own live key, so spending it later is correct and + # not an eviction. + _LOGGER.debug( + "Cancel key supplied for %s is our own live key: %s", + device_id, + request_uuid_to_use, + ) elif cached_uuid is not None and cached_uuid == request_uuid_to_use: # Ours, but aged: it matches, it is simply too old to vouch # for. Saying "does not match" here would be untrue, and the @@ -949,13 +1115,22 @@ async def async_stop_sound( ) try: - submitted = await self.api.async_stop_sound(device_id, request_uuid_to_use) - if not submitted: - self._note_push_transport_problem() - # No credential proof on this path: api.async_stop_sound - # swallows NovaAuthError and HTTP 401/403 into the same False - # as a timeout, so clearing the auth-failure state here would - # erase the very signal an expired sign-in produces. + stop_outcome = await self.api.async_stop_sound( + device_id, request_uuid_to_use + ) + # Same rule as on the play path above. The outer test stays in its + # negative form on purpose and is the single exception to the + # positive-list discipline: the safe default of THIS branch is + # FAILED, so an outcome nobody anticipated must fall through to it + # rather than be waved past. The cooldown inside it keeps the + # positive form, because there the safe default is to do nothing. + if stop_outcome is not SoundDispatchOutcome.ACCEPTED: + if stop_outcome is SoundDispatchOutcome.TRANSPORT_FAILED: + self._note_stop_transport_problem_without_extending() + # No credential proof on any non-accepted path: an auth + # rejection arrives as REJECTED_AUTH, and clearing the + # auth-failure state here would erase the very signal an expired + # sign-in produces. return StopSoundOutcome.FAILED # An accepted submission, and only that, proves credentials worked. self._set_auth_state(failed=False) @@ -1007,7 +1182,7 @@ async def async_stop_sound( conn_err, ) self.note_error(conn_err, where="async_stop_sound", device=device_id) - self._note_push_transport_problem() + self._note_stop_transport_problem_without_extending() return StopSoundOutcome.FAILED except Exception as err: _LOGGER.error( @@ -1017,5 +1192,8 @@ async def async_stop_sound( exc_info=True, ) self.note_error(err, where="async_stop_sound", device=device_id) - self._note_push_transport_problem() + # No cooldown, for the reason spelled out on the play path: + # api.async_stop_sound returns INTERNAL_ERROR for every unexpected + # Exception instead of raising, so this handler only ever sees a + # failure of our own bookkeeping around the call. return StopSoundOutcome.FAILED diff --git a/custom_components/googlefindmy/services.py b/custom_components/googlefindmy/services.py index acd2c5528..1e7ee4cc5 100644 --- a/custom_components/googlefindmy/services.py +++ b/custom_components/googlefindmy/services.py @@ -962,13 +962,15 @@ async def async_stop_sound_service(call: ServiceCall) -> None: translation_placeholders=placeholders, ) if outcome is StopSoundOutcome.FAILED: - # Distinct from SUPPRESSED: this one reached the transport and - # was refused. The api layer swallows every exception and - # returns False, so auth failures, 401/403, server errors, rate - # limits, network errors and empty replies all arrive here. - # "Try again in a moment" -- the suppressed advice -- is wrong - # for most of them, so this branch gets its own message and - # points at the log, which does carry the specific cause. + # Distinct from SUPPRESSED, and not by distance travelled: + # auth failures, 401/403, server rejections, rate limits, + # network errors, empty replies AND a missing action token all + # arrive here, because none of them is answered by waiting a + # moment. SUPPRESSED is reserved for the one condition that is + # -- a push transport that has not come up yet. See + # StopSoundOutcome.FAILED for the full rule. So this branch + # gets its own message and points at the log, which does carry + # the specific cause. raise _service_validation_error( "Stop sound for '{device_id}' was rejected".format(**placeholders), translation_key="stop_sound_rejected", diff --git a/docs/PLAY_SOUND_ARCHITECTURE.md b/docs/PLAY_SOUND_ARCHITECTURE.md index becdb17dc..abacdeb42 100644 --- a/docs/PLAY_SOUND_ARCHITECTURE.md +++ b/docs/PLAY_SOUND_ARCHITECTURE.md @@ -155,8 +155,106 @@ Two consequences follow, and both are already implemented: - Even `CANCELLED` claims only "submitted with a correlated cancel key", never "the device stopped". +**IRR-CA-SOUND-FAILURE-CLASS (who refused, and who may be blamed).** A sound +command can fail in ways that have nothing to do with each other, and the layer +that knows which one happened is `api.py`. `SoundDispatchOutcome` carries that +knowledge across the boundary instead of collapsing it into a bool: + +| Outcome | The server answered | May a caller arm a push cooldown | +|---|---|---| +| `ACCEPTED` | yes, HTTP 200 | no | +| `REJECTED_AUTH` | yes, on credentials (HTTP 401/403, or a permanent auth error) | no | +| `REJECTED_RATE_LIMIT` | yes, HTTP 429 | no | +| `REJECTED_SERVER` | yes, for any other reason (5xx, a Nova logic error, and the non-credential 4xx such as 400/404) | no | +| `TRANSPORT_FAILED` | no usable answer was obtained | **yes** (see the stop-path qualifier below) | +| `NOT_SENT` | no transport was used at all | no | +| `INTERNAL_ERROR` | our own defect or a broken contract | no | + +Only `TRANSPORT_FAILED` describes a broken push transport. Reporting anything +else as one is what let an expired sign-in, a rate limit and a bug of our own +each produce the same 90-second cooldown and the same `FcmStatus.DEGRADED`, and +`can_play_sound()` reported the button as unavailable for the duration. This is +the same failure class as `StopSoundOutcome` one layer up: a bool cannot carry +the state space. + +`PlaySoundResult` pairs that outcome with the cancel key, and the two are +deliberately independent. The key answers one question only, "may the device be +ringing" (see IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY); the cause is read from the +outcome and never inferred from the presence or absence of a key. + +The rule has exactly one consumer, and stating it in a type is not the same as +enforcing it. `coordinator/locate.py` holds every call to +`_note_push_transport_problem()` on the sound paths, and `async_play_sound` and +`async_stop_sound` each arm it on `TRANSPORT_FAILED` alone. On the stop path the +call is indirect and carries one further condition: it goes through +`_note_stop_transport_problem_without_extending()`, which reports the failure in +full but restores a window that was already running instead of restarting it +(IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN below). Their blanket +`except Exception` handlers do not arm it at all: `api.py` classifies every +unexpected exception in band and returns `INTERNAL_ERROR` rather than raising +(`api.py`, the final handler of both sound methods), so what still reaches those +coordinator handlers is a failure of the coordinator's own bookkeeping around +the call, and none of that is the push transport. The two typed handlers for +`TimeoutError`/`ClientConnectionError`/`ClientError` do keep the cooldown: `api` +is a Protocol, and an aiohttp error surfacing from an implementation that does +not wrap it really is a transport failure. The `pytest.mark.parametrize` tables +in `tests/test_coordinator_locate_basics.py` carry one row per enum member and +are guarded against a member being added without a decision, so the "may a +caller arm a push cooldown" column above is a measured claim, not a wish. + +**IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN (the window must not silence its +own remedy).** A play that reaches the wire and then loses the answer does two +things in one breath: it stores a cancel key, and it correctly arms the +90-second push cooldown. `_api_push_ready()` short-circuits on that cooldown, so +the stop the key exists for used to be suppressed for the first 90 seconds after +that play, which is exactly when a user reaches for the Stop button. (How long +the ring itself lasts is a different timer on a different layer, and nothing +here claims to know it -- see the note on the aged cached key in +`async_stop_sound`.) `async_stop_sound` therefore passes the readiness +gate in exactly one case: a window is running **and** the stop would be +correlated, that is, it carries our own cached key and that key is still fresh. + +The polarity is deliberately the opposite of `can_play_sound()` and the manual +locate guard, which treat a running window as the hard block and an unconfirmed +transport as passable. Two limits keep the exception honest: + +- **What it can distinguish.** While a window runs, `_api_push_ready()` never + asks the transport, so the transport state is unknown. The guard separates + "not ready because a window is running" from "not ready for another reason" + and nothing more; a transport failure is the only thing that arms the window, + so the two overlap by construction. +- **It cannot feed itself.** A broken-through stop that fails on the transport + must not restart the window. `_note_push_transport_problem()` sets + `_push_cooldown_until` absolutely, and the stop button has no availability + guard, so an unguarded call would let repeated presses keep Play Sound and + manual locate disabled indefinitely. `async_stop_sound` routes both of its + cooldown calls through + `_note_stop_transport_problem_without_extending()`, which arms a window only + when none is running. + +`_stop_would_be_correlated()` is the single definition of "correlated" behind +both this gate and the decision to spend the cached key +(IRR-CA-POP-ON-CORRELATED-CANCEL-ONLY); a second inline derivation at either +site is the drift the extraction exists to prevent, and +`tests/test_coordinator_locate_basics.py` pins both sites to it. + +The exception is not free, and the price is a changed outcome class rather than +a changed guarantee: a stop that used to end as `SUPPRESSED` now reaches the +transport and can end as `FAILED`, which `services.py` reports with a different +translation key (`stop_sound_rejected` instead of `stop_sound_suppressed`). It +also removes, for the duration of a window, the only rate brake that sat in +front of the stop dispatch -- outside a window there never was one, and a +`REJECTED_RATE_LIMIT` answer deliberately arms no cooldown. + #### Follow-up (not in this change) +The play path arms the same cooldown without this guard +(`coordinator/locate.py`, `async_play_sound`), and `can_play_sound()` returns +early on a cached `can_ring` capability before it ever looks at the window, so a +play can reach the arming call while a window runs. That behaviour predates the +stop-path exception and is unchanged by it; guarding it is a separate decision +with its own tests. + Closing the boundary on the cloud path means wiring **Path B**: registering an FCM callback for sound events and matching the incoming `ExecuteActionRequestMetadata.requestUuid` against the key held by the diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index f3fd1322a..59c72f8b0 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -3,6 +3,7 @@ from __future__ import annotations +import sys from functools import lru_cache from importlib.util import module_from_spec, spec_from_file_location from pathlib import Path @@ -32,7 +33,22 @@ def load_googlefindmy_const_module() -> ModuleType: raise RuntimeError("Unable to load googlefindmy const module") module = module_from_spec(spec) - spec.loader.exec_module(module) + # Register before exec_module, as the importlib recipe for "importing a source + # file directly" prescribes. Skipping this half of the recipe worked only as + # long as const.py contained nothing that resolves its own annotations at + # class-creation time. @dataclass does: with `from __future__ import + # annotations` every field annotation is a string, and dataclasses resolves it + # via sys.modules[cls.__module__] to tell a KW_ONLY/ClassVar marker from a real + # field. Unregistered, that lookup yields None and the module fails to import + # with `AttributeError: 'NoneType' object has no attribute '__dict__'`, which + # this conftest turns into a collection error for the entire test suite. + sys.modules[spec.name] = module + try: + spec.loader.exec_module(module) + except BaseException: + # Do not leave a half-initialised module behind for the next importer. + sys.modules.pop(spec.name, None) + raise return module diff --git a/tests/helpers/locate_mixin_stub.py b/tests/helpers/locate_mixin_stub.py index 9a24bac3d..c4d0732eb 100644 --- a/tests/helpers/locate_mixin_stub.py +++ b/tests/helpers/locate_mixin_stub.py @@ -19,6 +19,10 @@ from typing import Any from unittest.mock import AsyncMock, MagicMock +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, +) from custom_components.googlefindmy.coordinator.locate import LocateOperations @@ -73,8 +77,12 @@ def __init__( self.min_poll_interval = min_poll_interval self.api = api if api is not None else MagicMock() self.api.async_get_device_location = AsyncMock(return_value={}) - self.api.async_play_sound = AsyncMock(return_value=(True, "uuid-stub")) - self.api.async_stop_sound = AsyncMock(return_value=True) + self.api.async_play_sound = AsyncMock( + return_value=PlaySoundResult(SoundDispatchOutcome.ACCEPTED, "uuid-stub") + ) + self.api.async_stop_sound = AsyncMock( + return_value=SoundDispatchOutcome.ACCEPTED + ) # Cross-mixin methods owned by other mixins or DataUpdateCoordinator: # mock them so default behavior is a no-op. Tests override per-case. diff --git a/tests/test_api_basics.py b/tests/test_api_basics.py index d2c3c4c72..5b72db4c0 100644 --- a/tests/test_api_basics.py +++ b/tests/test_api_basics.py @@ -39,6 +39,7 @@ CONTRIBUTOR_MODE_HIGH_TRAFFIC, CONTRIBUTOR_MODE_IN_ALL_AREAS, DEFAULT_CONTRIBUTOR_MODE, + SoundDispatchOutcome, ) from custom_components.googlefindmy.exceptions import MissingTokenCacheError from custom_components.googlefindmy.NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( @@ -1215,7 +1216,8 @@ def test_missing_token_short_circuits(self) -> None: # PRE-dispatch guard: nothing was sent, so there is no cancel key to keep. api_module._FCM_ReceiverGetter = None api = GoogleFindMyAPI(cache=StubCache(entry_id="e")) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) def test_empty_submission_drops_cancel_key( self, monkeypatch: pytest.MonkeyPatch @@ -1227,7 +1229,8 @@ def test_empty_submission_drops_cancel_key( api = self._api_with_token(monkeypatch) self._patch_generate_uuid(monkeypatch) self._patch_submit(monkeypatch, None) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) def test_success_returns_injected_uuid( self, monkeypatch: pytest.MonkeyPatch @@ -1244,7 +1247,8 @@ async def _echo_submit(*_a: Any, **_k: Any) -> Any: monkeypatch.setattr( api_module, "async_submit_start_sound_request", _echo_submit ) - assert run_coro(api.async_play_sound("d")) == (True, "uuid-injected") + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (True, "uuid-injected") @pytest.mark.parametrize( "exc", @@ -1283,7 +1287,8 @@ def test_any_submitter_exception_drops_cancel_key( api = self._api_with_token(monkeypatch) self._patch_generate_uuid(monkeypatch) self._patch_submit(monkeypatch, None, raises=exc) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) def test_post_dispatch_network_failure_keeps_cancel_key( self, monkeypatch: pytest.MonkeyPatch @@ -1299,7 +1304,8 @@ def test_post_dispatch_network_failure_keeps_cancel_key( err = NovaError("network failed after retries") err.dispatched = True self._patch_submit(monkeypatch, None, raises=err) - assert run_coro(api.async_play_sound("d")) == (False, "uuid-injected") + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, "uuid-injected") def test_pre_dispatch_network_failure_drops_cancel_key( self, monkeypatch: pytest.MonkeyPatch @@ -1312,7 +1318,204 @@ def test_pre_dispatch_network_failure_drops_cancel_key( err = NovaError("connect failed before the wire") err.dispatched = False self._patch_submit(monkeypatch, None, raises=err) - assert run_coro(api.async_play_sound("d")) == (False, None) + result = run_coro(api.async_play_sound("d")) + assert (result.accepted, result.cancel_key) == (False, None) + + @pytest.mark.parametrize( + ("raised", "expected"), + [ + (NovaAuthError(401, "expired"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaAuthError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), + # NovaAuthError is raised for EVERY non-retryable 4xx, not only for + # credential rejections: nova_request.py names "403 Forbidden, 404 + # Not Found" in the very comment above the raise, and + # HTTP_RETRY_ELIGIBLE holds no 4xx besides 408 and 429. A missing + # device or a malformed request must therefore not be reported as + # REJECTED_AUTH, whose contract is "refused on credentials". + ( + NovaAuthError(404, "no such device"), + SoundDispatchOutcome.REJECTED_SERVER, + ), + (NovaAuthError(400, "bad request"), SoundDispatchOutcome.REJECTED_SERVER), + # Permanence outranks the status code: the subclass exists to say + # "re-authentication is definitively required", so it stays AUTH + # even if it ever carries a status outside 401/403. + ( + api_module.NovaAuthPermanentError(404, "aas rejected"), + SoundDispatchOutcome.REJECTED_AUTH, + ), + (NovaHTTPError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaHTTPError(503, "unavailable"), SoundDispatchOutcome.REJECTED_SERVER), + (NovaRateLimitError("slow down"), SoundDispatchOutcome.REJECTED_RATE_LIMIT), + (NovaLogicError(3, "logic"), SoundDispatchOutcome.REJECTED_SERVER), + ( + NovaProtobufDecodeError("garbage"), + SoundDispatchOutcome.INTERNAL_ERROR, + ), + (NovaError("socket died"), SoundDispatchOutcome.TRANSPORT_FAILED), + (ClientError("pre-dispatch"), SoundDispatchOutcome.TRANSPORT_FAILED), + (ValueError("our own bug"), SoundDispatchOutcome.INTERNAL_ERROR), + ], + ) + def test_play_sound_classifies_every_exit( + self, + monkeypatch: pytest.MonkeyPatch, + raised: Exception, + expected: SoundDispatchOutcome, + ) -> None: + """Every exit of async_play_sound must name its own cause. + + A server saying no, a network that never answered and a bug on our side + used to be the same ``(False, None)``. The coordinator read that as a + push transport problem and armed a 90-second cooldown for all three. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None, raises=raised) + assert run_coro(api.async_play_sound("d")).outcome is expected + + def test_a_non_auth_4xx_does_not_log_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """The log line IS the user-visible effect of this classification. + + Both outcomes reach the same ``StopSoundOutcome.FAILED`` and the same + ``stop_sound_rejected`` message downstream, so nothing but the log tells + the user whether to check their sign-in or their device list. Pinning + only the returned enum would leave the whole point of the split + unguarded: the branch could collapse back to a single + ``_LOGGER.error("Authentication failed ...")`` with every enum + assertion still green. Mirrors the "assert both the warning log and the + exception" rule in ``tests/AGENTS.md``. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(404, "gone")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_play_sound("d")).outcome + is SoundDispatchOutcome.REJECTED_SERVER + ) + assert "Client error (HTTP 404)" in caplog.text + assert "Authentication failed" not in caplog.text + # The level is part of the user-visible effect: HA's log panel and its + # error counter treat ERROR differently from WARNING, so a silent + # promotion would put a deleted device back among the alarms. + levels = { + r.levelno for r in caplog.records if "Client error (HTTP 404)" in r.message + } + assert levels == {logging.WARNING} + + def test_a_credential_rejection_still_logs_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """The counterpart: 403 must keep naming credentials. + + Without this row the previous test is satisfied by a branch that never + says "Authentication failed" at all, which would bury a real expired + sign-in instead of the reverse. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(403, "denied")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_play_sound("d")).outcome + is SoundDispatchOutcome.REJECTED_AUTH + ) + assert "Authentication failed" in caplog.text + assert "Client error (HTTP 403)" not in caplog.text + levels = { + r.levelno for r in caplog.records if "Authentication failed" in r.message + } + assert levels == {logging.ERROR} + + def test_auth_error_without_a_readable_status_stays_auth( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A NovaAuthError whose status cannot be read keeps the old verdict. + + Every real instance carries one (the constructor takes it positionally), + so this is the test-double and future-subclass case. The conservative + reading of a type named "auth" is auth, and pinning it here keeps the + fallback from being a claim in a comment. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + err = NovaAuthError(404, "double without a status") + # delattr, not `= None`: the annotation says int, and removing the + # instance attribute is exactly what makes getattr fall back. + delattr(err, "status") + self._patch_submit(monkeypatch, None, raises=err) + assert ( + run_coro(api.async_play_sound("d")).outcome + is SoundDispatchOutcome.REJECTED_AUTH + ) + + def test_missing_action_token_is_not_sent(self) -> None: + """No FCM token means no transport was used, so nobody may be blamed.""" + + api_module._FCM_ReceiverGetter = None + api = GoogleFindMyAPI(cache=StubCache(entry_id="e")) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.NOT_SENT + assert result.cancel_key is None + + def test_empty_submitter_reply_is_internal_error( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The submitter returns a tuple on 200 and re-raises otherwise. + + A None therefore breaks its own contract: our bug, not an outage. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + self._patch_submit(monkeypatch, None) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.INTERNAL_ERROR + assert result.cancel_key is None + + def test_http_200_is_accepted(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Acceptance carries the cancel key, and only acceptance sets accepted.""" + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + + async def _echo_submit(*_a: Any, **_k: Any) -> Any: + return ("AB", _k.get("request_uuid")) + + monkeypatch.setattr( + api_module, "async_submit_start_sound_request", _echo_submit + ) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.ACCEPTED + assert result.accepted is True + assert result.cancel_key == "uuid-injected" + + def test_dispatched_transport_failure_keeps_key_and_names_the_transport( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Outcome and cancel key are two independent facts, not one. + + The key answers "may the device be ringing", the outcome answers "who + refused". Reading the cause off the key is the out-of-band channel that + PlaySoundResult replaces. + """ + + api = self._api_with_token(monkeypatch) + self._patch_generate_uuid(monkeypatch) + err = NovaError("network failed after retries") + err.dispatched = True + self._patch_submit(monkeypatch, None, raises=err) + result = run_coro(api.async_play_sound("d")) + assert result.outcome is SoundDispatchOutcome.TRANSPORT_FAILED + assert result.cancel_key == "uuid-injected" + assert result.accepted is False class TestAsyncStopSoundErrorMapping: @@ -1340,17 +1543,26 @@ async def _submit(*_a: Any, **_k: Any) -> Any: def test_missing_token_short_circuits(self) -> None: api_module._FCM_ReceiverGetter = None api = GoogleFindMyAPI(cache=StubCache(entry_id="e")) - assert run_coro(api.async_stop_sound("d")) is False + # No transport was used, so neither the server nor the network is at fault. + assert run_coro(api.async_stop_sound("d")) is SoundDispatchOutcome.NOT_SENT def test_none_response_returns_false(self, monkeypatch: pytest.MonkeyPatch) -> None: api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, None) - assert run_coro(api.async_stop_sound("d", "uuid-1234")) is False + # The submitter re-raises on every non-acceptance, so an empty reply is a + # broken contract on our side, not an outage. + assert ( + run_coro(api.async_stop_sound("d", "uuid-1234")) + is SoundDispatchOutcome.INTERNAL_ERROR + ) def test_success_returns_true(self, monkeypatch: pytest.MonkeyPatch) -> None: api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, "CDEF") - assert run_coro(api.async_stop_sound("d", "uuid-5678")) is True + assert ( + run_coro(api.async_stop_sound("d", "uuid-5678")) + is SoundDispatchOutcome.ACCEPTED + ) def test_stop_without_uuid_does_not_claim_success( self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture @@ -1367,7 +1579,7 @@ def test_stop_without_uuid_does_not_claim_success( self._patch_submit(monkeypatch, "CDEF") with caplog.at_level(logging.DEBUG): - assert run_coro(api.async_stop_sound("d")) is True + assert run_coro(api.async_stop_sound("d")) is SoundDispatchOutcome.ACCEPTED assert "successfully" not in caplog.text warnings = [ @@ -1387,7 +1599,10 @@ def test_stop_with_uuid_logs_the_cancel_key_branch( self._patch_submit(monkeypatch, "CDEF") with caplog.at_level(logging.DEBUG): - assert run_coro(api.async_stop_sound("d", "uuid-5678")) is True + assert ( + run_coro(api.async_stop_sound("d", "uuid-5678")) + is SoundDispatchOutcome.ACCEPTED + ) assert "cancel key present" in caplog.text assert "without a cancel key" not in caplog.text @@ -1412,25 +1627,108 @@ def test_blank_uuid_logs_as_uncorrelated_not_as_a_key( self._patch_submit(monkeypatch, "CDEF") with caplog.at_level(logging.DEBUG): - assert run_coro(api.async_stop_sound("d", blank)) is True + assert ( + run_coro(api.async_stop_sound("d", blank)) + is SoundDispatchOutcome.ACCEPTED + ) assert "cancel key present" not in caplog.text assert "without a cancel key" in caplog.text @pytest.mark.parametrize( - "exc", + ("exc", "expected"), [ - NovaAuthError(401, "auth"), - NovaHTTPError(403, "http"), - NovaHTTPError(500, "http"), - NovaRateLimitError("rate"), - ClientError("net"), - Exception("boom"), + (NovaAuthError(401, "auth"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaAuthError(403, "forbidden"), SoundDispatchOutcome.REJECTED_AUTH), + # Same rule as on the play path: the exception type is wider than + # its name, so the status decides. + ( + NovaAuthError(404, "no such device"), + SoundDispatchOutcome.REJECTED_SERVER, + ), + (NovaAuthError(400, "bad request"), SoundDispatchOutcome.REJECTED_SERVER), + ( + api_module.NovaAuthPermanentError(404, "aas rejected"), + SoundDispatchOutcome.REJECTED_AUTH, + ), + (NovaHTTPError(403, "http"), SoundDispatchOutcome.REJECTED_AUTH), + (NovaHTTPError(500, "http"), SoundDispatchOutcome.REJECTED_SERVER), + (NovaRateLimitError("rate"), SoundDispatchOutcome.REJECTED_RATE_LIMIT), + (NovaLogicError(3, "logic"), SoundDispatchOutcome.REJECTED_SERVER), + ( + NovaProtobufDecodeError("garbage"), + SoundDispatchOutcome.INTERNAL_ERROR, + ), + (NovaError("socket died"), SoundDispatchOutcome.TRANSPORT_FAILED), + (ClientError("net"), SoundDispatchOutcome.TRANSPORT_FAILED), + (Exception("boom"), SoundDispatchOutcome.INTERNAL_ERROR), ], ) - def test_documented_exceptions_return_false( - self, monkeypatch: pytest.MonkeyPatch, exc: BaseException + def test_documented_exceptions_are_classified( + self, + monkeypatch: pytest.MonkeyPatch, + exc: BaseException, + expected: SoundDispatchOutcome, ) -> None: + """Stop carries the same classification contract as Play. + + None of these is an acceptance, but only the two transport rows may make + a caller arm a push cooldown. Before the contract existed all nine + collapsed into a single ``False``. + """ + api = self._api_with_token(monkeypatch) self._patch_submit(monkeypatch, None, raises=exc) - assert run_coro(api.async_stop_sound("d")) is False + assert run_coro(api.async_stop_sound("d")) is expected + + def test_a_non_auth_4xx_does_not_log_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """Same log guard as on the play path, and for the same reason.""" + + api = self._api_with_token(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(404, "gone")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_stop_sound("d")) + is SoundDispatchOutcome.REJECTED_SERVER + ) + assert "Client error (HTTP 404)" in caplog.text + assert "Authentication failed" not in caplog.text + # The level is part of the user-visible effect: HA's log panel and its + # error counter treat ERROR differently from WARNING, so a silent + # promotion would put a deleted device back among the alarms. + levels = { + r.levelno for r in caplog.records if "Client error (HTTP 404)" in r.message + } + assert levels == {logging.WARNING} + + def test_a_credential_rejection_still_logs_an_authentication_failure( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + """The counterpart on the stop path.""" + + api = self._api_with_token(monkeypatch) + self._patch_submit(monkeypatch, None, raises=NovaAuthError(403, "denied")) + with caplog.at_level(logging.DEBUG): + assert ( + run_coro(api.async_stop_sound("d")) + is SoundDispatchOutcome.REJECTED_AUTH + ) + assert "Authentication failed" in caplog.text + assert "Client error (HTTP 403)" not in caplog.text + levels = { + r.levelno for r in caplog.records if "Authentication failed" in r.message + } + assert levels == {logging.ERROR} + + def test_auth_error_without_a_readable_status_stays_auth( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Same fallback as on the play path, pinned on both sides.""" + + api = self._api_with_token(monkeypatch) + err = NovaAuthError(404, "double without a status") + delattr(err, "status") + self._patch_submit(monkeypatch, None, raises=err) + assert run_coro(api.async_stop_sound("d")) is SoundDispatchOutcome.REJECTED_AUTH diff --git a/tests/test_api_fcm_token_scoping.py b/tests/test_api_fcm_token_scoping.py index 93b159484..29cc579d7 100644 --- a/tests/test_api_fcm_token_scoping.py +++ b/tests/test_api_fcm_token_scoping.py @@ -10,6 +10,7 @@ from custom_components.googlefindmy import api as api_module from custom_components.googlefindmy.api import GoogleFindMyAPI +from custom_components.googlefindmy.const import SoundDispatchOutcome @dataclass @@ -137,14 +138,22 @@ async def fake_submit_stop( ) async def _exercise() -> None: - ok1, uuid1 = await api_entry_1.async_play_sound("device-1") - ok2, uuid2 = await api_entry_2.async_play_sound("device-2") + play1 = await api_entry_1.async_play_sound("device-1") + play2 = await api_entry_2.async_play_sound("device-2") + ok1, uuid1 = play1.accepted, play1.cancel_key + ok2, uuid2 = play2.accepted, play2.cancel_key assert ok1 and ok2 # Each play generates its own client-side cancel key (non-None, distinct). assert uuid1 and uuid2 assert uuid1 != uuid2 - assert await api_entry_1.async_stop_sound("device-1") - assert await api_entry_2.async_stop_sound("device-2") + assert ( + await api_entry_1.async_stop_sound("device-1") + is SoundDispatchOutcome.ACCEPTED + ) + assert ( + await api_entry_2.async_stop_sound("device-2") + is SoundDispatchOutcome.ACCEPTED + ) asyncio.run(_exercise()) @@ -223,10 +232,14 @@ async def fake_submit_stop( monkeypatch.setattr(api, "_get_fcm_token_for_action", lambda: "tok-uuid") async def _exercise() -> None: - success, request_uuid = await api.async_play_sound("device-uuid") + play = await api.async_play_sound("device-uuid") + success, request_uuid = play.accepted, play.cancel_key assert success is True assert request_uuid == "uuid-device-uuid" - assert await api.async_stop_sound("device-uuid", request_uuid) + assert ( + await api.async_stop_sound("device-uuid", request_uuid) + is SoundDispatchOutcome.ACCEPTED + ) asyncio.run(_exercise()) diff --git a/tests/test_api_location_selection.py b/tests/test_api_location_selection.py index 0779d7ee5..f833d61a7 100644 --- a/tests/test_api_location_selection.py +++ b/tests/test_api_location_selection.py @@ -11,6 +11,10 @@ import custom_components.googlefindmy.api as api_module from custom_components.googlefindmy.api import GoogleFindMyAPI +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, +) from tests.helpers import drain_loop @@ -53,15 +57,15 @@ async def async_get_device_location( self.calls.append(("loc", (device_id, device_name))) return {"id": device_id, "name": device_name} - async def async_play_sound(self, device_id: str) -> tuple[bool, str | None]: + async def async_play_sound(self, device_id: str) -> PlaySoundResult: self.calls.append(("play", (device_id,))) - return True, "uuid-play" + return PlaySoundResult(SoundDispatchOutcome.ACCEPTED, "uuid-play") async def async_stop_sound( self, device_id: str, request_uuid: str | None = None - ) -> bool: + ) -> SoundDispatchOutcome: self.calls.append(("stop", (device_id, request_uuid))) - return True + return SoundDispatchOutcome.ACCEPTED class _LoopCaptureHarness(GoogleFindMyAPI): diff --git a/tests/test_coordinator_locate_basics.py b/tests/test_coordinator_locate_basics.py index a7e725223..714ebc456 100644 --- a/tests/test_coordinator_locate_basics.py +++ b/tests/test_coordinator_locate_basics.py @@ -14,13 +14,17 @@ import asyncio import math +import time from unittest.mock import MagicMock import pytest +from aiohttp import ClientConnectionError from homeassistant.exceptions import HomeAssistantError from custom_components.googlefindmy.const import ( DEFAULT_MIN_POLL_INTERVAL, + PlaySoundResult, + SoundDispatchOutcome, StopSoundOutcome, ) from custom_components.googlefindmy.NovaApi.ExecuteAction.LocateTracker.decrypt_locations import ( @@ -360,6 +364,20 @@ async def test_decrypt_failure_at_threshold_starts_reauth( coord.config_entry.async_start_reauth.assert_called_once() +# One row per ``SoundDispatchOutcome`` member: (outcome, accepted, may arm the +# push cooldown). Kept at module level so the exhaustiveness guard below reads +# the same table the parametrisation runs on. +PLAY_OUTCOME_CASES: list[tuple[SoundDispatchOutcome, bool, bool]] = [ + (SoundDispatchOutcome.ACCEPTED, True, False), + (SoundDispatchOutcome.REJECTED_AUTH, False, False), + (SoundDispatchOutcome.REJECTED_RATE_LIMIT, False, False), + (SoundDispatchOutcome.REJECTED_SERVER, False, False), + (SoundDispatchOutcome.NOT_SENT, False, False), + (SoundDispatchOutcome.INTERNAL_ERROR, False, False), + (SoundDispatchOutcome.TRANSPORT_FAILED, False, True), +] + + class TestAsyncPlaySoundGating: """Exercise gating branches of ``async_play_sound``.""" @@ -378,23 +396,178 @@ async def test_success_stores_uuid(self, coord: LocateStub) -> None: async def test_failure_notes_problem(self, coord: LocateStub) -> None: coord._device_caps["dev-1"] = {"can_ring": True} - coord.api.async_play_sound.return_value = (False, None) + coord.api.async_play_sound.return_value = PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED + ) ok = await coord.async_play_sound("dev-1") assert ok is False coord._note_push_transport_problem.assert_called_once() - async def test_unexpected_exception_returns_false(self, coord: LocateStub) -> None: + async def test_unexpected_exception_is_not_a_transport_problem( + self, coord: LocateStub + ) -> None: + """A bug of our own must not be reported as a broken push transport. + + ``api.async_play_sound`` classifies every ``Exception`` in band and + returns ``INTERNAL_ERROR`` instead of raising, so nothing that reaches + this handler came from the push transport: what is left is the + coordinator's own body around the call, or an ``api`` implementation + that breaks the Protocol. Arming the push cooldown for either is the + self-inflicted outage this contract was written to stop. The error is + still recorded. + """ + coord._device_caps["dev-1"] = {"can_ring": True} coord.api.async_play_sound.side_effect = RuntimeError("boom") ok = await coord.async_play_sound("dev-1") assert ok is False coord.note_error.assert_called_once() - coord._note_push_transport_problem.assert_called_once() + coord._note_push_transport_problem.assert_not_called() + + async def test_failed_play_does_not_clear_auth_state( + self, coord: LocateStub + ) -> None: + """A play that was not accepted must not vouch for the credentials. + + ``api.async_play_sound`` collapses a 401/403 rejection into the same + ``(False, None)`` as a timeout, so clearing the auth-failure state here + deleted the signal an expired sign-in produces. The stop path never did + this (see ``async_stop_sound``); the two paths now agree. + """ + + coord._device_caps["dev-1"] = {"can_ring": True} + coord.api.async_play_sound.return_value = PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED + ) + + assert await coord.async_play_sound("dev-1") is False + + coord._set_auth_state.assert_not_called() + + async def test_accepted_play_still_clears_auth_state( + self, coord: LocateStub + ) -> None: + """The positive half of the rule must not be lost with the fix.""" + + coord._device_caps["dev-1"] = {"can_ring": True} + + assert await coord.async_play_sound("dev-1") is True + + coord._set_auth_state.assert_called_once_with(failed=False) + + @pytest.mark.parametrize( + ("outcome", "expect_accepted", "expect_cooldown"), PLAY_OUTCOME_CASES + ) + async def test_only_a_transport_failure_arms_the_push_cooldown( + self, + coord: LocateStub, + outcome: SoundDispatchOutcome, + expect_accepted: bool, + expect_cooldown: bool, + ) -> None: + """A server saying no is not a network outage. + + Every non-acceptance used to arrive as a plain ``False``, so all of them + armed the 90-second push cooldown, flipped the integration to + ``FcmStatus.DEGRADED`` and made ``can_play_sound`` report the button as + unavailable. ``SoundDispatchOutcome`` names the cause; only a transport + that never gave us a usable answer may arm that cooldown. The list is + exhaustive over the enum on purpose: a new member added without a + decision here shows up as a missing parametrisation, not as a silent + default. + """ + + coord._device_caps["dev-1"] = {"can_ring": True} + coord.api.async_play_sound.return_value = PlaySoundResult(outcome) + + assert await coord.async_play_sound("dev-1") is expect_accepted + + assert coord._note_push_transport_problem.called is expect_cooldown + + def test_the_play_parametrisation_covers_every_outcome(self) -> None: + """Guard the exhaustiveness the case table claims for itself. + + The decision "which outcome may arm the cooldown" has to be taken for + every member of the enum. A member added later without a row here would + otherwise silently inherit whatever the ``if`` cascade happens to do. + """ + + assert {case[0] for case in PLAY_OUTCOME_CASES} == set(SoundDispatchOutcome) + + +# One row per ``SoundDispatchOutcome`` member on the stop side: (dispatch, +# resulting StopSoundOutcome, may arm the push cooldown, may vouch for the +# credentials). ACCEPTED lands on UNCORRELATED here because the key is passed +# in by the caller and is none of ours -- that split is pinned by its own tests. +STOP_OUTCOME_CASES: list[tuple[SoundDispatchOutcome, StopSoundOutcome, bool, bool]] = [ + (SoundDispatchOutcome.ACCEPTED, StopSoundOutcome.UNCORRELATED, False, True), + (SoundDispatchOutcome.REJECTED_AUTH, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.REJECTED_RATE_LIMIT, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.REJECTED_SERVER, StopSoundOutcome.FAILED, False, False), + # NOT_SENT is FAILED, not SUPPRESSED: it never reached the wire, but a + # receiver that is up and yields no action token is not answered by + # waiting. StopSoundOutcome.FAILED states the rule; this row enforces it. + (SoundDispatchOutcome.NOT_SENT, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.INTERNAL_ERROR, StopSoundOutcome.FAILED, False, False), + (SoundDispatchOutcome.TRANSPORT_FAILED, StopSoundOutcome.FAILED, True, False), +] class TestAsyncStopSoundGating: """Exercise gating branches of ``async_stop_sound``.""" + @pytest.mark.parametrize( + ("dispatch", "expect_outcome", "expect_cooldown", "expect_auth_cleared"), + STOP_OUTCOME_CASES, + ) + async def test_only_a_transport_failure_arms_the_push_cooldown( + self, + coord: LocateStub, + dispatch: SoundDispatchOutcome, + expect_outcome: StopSoundOutcome, + expect_cooldown: bool, + expect_auth_cleared: bool, + ) -> None: + """The same rule as on the play path, on the stop path. + + A stop the server refused on credentials, refused outright or rate + limited reached this method as a plain ``False`` before the contract + existed, so each of them armed the 90-second cooldown -- which then + suppressed the user's next attempt for a minute and a half over a + problem the network never had. + """ + + coord.api.async_stop_sound.return_value = dispatch + + outcome = await coord.async_stop_sound("dev-1", request_uuid="foreign-key") + + assert outcome is expect_outcome + assert coord._note_push_transport_problem.called is expect_cooldown + assert coord._set_auth_state.called is expect_auth_cleared + + def test_the_stop_parametrisation_covers_every_outcome(self) -> None: + """Guard the exhaustiveness the case table claims for itself.""" + + assert {case[0] for case in STOP_OUTCOME_CASES} == set(SoundDispatchOutcome) + + async def test_unexpected_exception_is_not_a_transport_problem( + self, coord: LocateStub + ) -> None: + """Mirror of the play-path rule: our own bug is not an outage. + + ``api.async_stop_sound`` returns ``INTERNAL_ERROR`` for every unexpected + ``Exception`` instead of raising, so this handler only sees failures of + the coordinator's own bookkeeping around the call. + """ + + coord.api.async_stop_sound.side_effect = RuntimeError("boom") + + outcome = await coord.async_stop_sound("dev-1", request_uuid="x") + + assert outcome is StopSoundOutcome.FAILED + coord.note_error.assert_called_once() + coord._note_push_transport_problem.assert_not_called() + async def test_blocks_when_push_not_ready(self, coord: LocateStub) -> None: coord._api_push_ready.return_value = False outcome = await coord.async_stop_sound("dev-1") @@ -410,13 +583,14 @@ async def test_rejected_submission_is_failed_not_suppressed( ) -> None: """A stop the transport refused must not claim a local, transient cause. - ``api.async_stop_sound`` swallows every exception and returns False, so - auth failures, 401/403, server errors, rate limits and network errors - all arrive as a plain False. Reporting them as SUPPRESSED would tell a - user with an expired sign-in to wait a moment. + ``api.async_stop_sound`` swallows every exception and returns a + ``SoundDispatchOutcome`` instead of raising, so auth failures, 401/403, + server errors, rate limits and network errors all arrive as a + non-ACCEPTED value here. Reporting them as SUPPRESSED would tell a user + with an expired sign-in to wait a moment. """ - coord.api.async_stop_sound.return_value = False + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED outcome = await coord.async_stop_sound("dev-1") assert outcome is StopSoundOutcome.FAILED coord.api.async_stop_sound.assert_awaited_once() @@ -469,7 +643,7 @@ async def test_missing_uuid_reports_uncorrelated(self, coord: LocateStub) -> Non coord.api.async_stop_sound.assert_awaited_once_with("dev-1", None) async def test_failure_notes_problem(self, coord: LocateStub) -> None: - coord.api.async_stop_sound.return_value = False + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED outcome = await coord.async_stop_sound("dev-1", request_uuid="x") assert outcome is StopSoundOutcome.FAILED coord._note_push_transport_problem.assert_called_once() @@ -480,10 +654,356 @@ async def test_failed_stop_keeps_a_fresh_cancel_key( """IRR-CA-CANCEL-KEY-ON-SUCCESS-ONLY: a rejected stop spends nothing.""" coord._sound_request_uuids["dev-1"] = "cached-uuid" - coord.api.async_stop_sound.return_value = False + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED outcome = await coord.async_stop_sound("dev-1") assert outcome is StopSoundOutcome.FAILED assert coord._sound_request_uuids["dev-1"] == "cached-uuid" +class TestStopBreaksSelfInflictedCooldown: + """AP-5 / F2: the cancel key a failed play preserved must be usable at once. + + A play that reached the wire and then lost the answer does two things in the + same breath: it stores a cancel key, and it arms the 90-second push cooldown + (correctly, because that IS a transport failure). ``_api_push_ready()`` + short-circuits to False while the cooldown runs, so the stop that the key + exists for was suppressed for the first 90 seconds after that play, which is + exactly when a user reaches for the Stop button. (How long the ring itself + lasts is a different timer on a different layer and is not claimed here.) + See IRR-CA-STOP-BREAKS-SELF-INFLICTED-COOLDOWN. + + The exception is deliberately narrow. It applies only when the stop would be + correlated (our own, fresh cancel key); a stop that would report + UNCORRELATED buys nothing, so the anti-spam purpose of the cooldown is kept + for every case that has no provable benefit. It is not free either: a stop + that used to end as SUPPRESSED now reaches the transport and can end as + FAILED instead, which is a different service-level message. That change of + outcome class is pinned below rather than left implicit. + """ + + async def test_ambiguous_play_does_not_block_the_following_stop( + self, coord: LocateStub + ) -> None: + """The whole F2 chain, end to end: play loses the answer, stop follows.""" + + coord._device_caps["dev-1"] = {"can_ring": True} + + def _note(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_note) + coord._api_push_ready = MagicMock( + side_effect=lambda: time.monotonic() >= coord._push_cooldown_until + ) + coord.api.async_play_sound.return_value = PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, cancel_key="uuid-ambiguous" + ) + + assert await coord.async_play_sound("dev-1") is False + assert coord._sound_request_uuids.get("dev-1") == "uuid-ambiguous" + assert coord._push_cooldown_until > time.monotonic() + + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.ACCEPTED + outcome = await coord.async_stop_sound("dev-1") + + assert outcome is StopSoundOutcome.CANCELLED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-ambiguous") + + async def test_explicit_own_fresh_key_breaks_the_cooldown( + self, coord: LocateStub + ) -> None: + """The service handler passes the key explicitly; same right to be sent.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + outcome = await coord.async_stop_sound("dev-1", request_uuid="uuid-fresh") + + assert outcome is StopSoundOutcome.CANCELLED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + + async def test_blank_explicit_key_falls_back_to_the_cached_key( + self, coord: LocateStub + ) -> None: + """A blank argument means "no opinion" here too, not "no key".""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + outcome = await coord.async_stop_sound("dev-1", request_uuid=" ") + + assert outcome is StopSoundOutcome.CANCELLED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + + async def test_a_broken_through_stop_that_fails_does_not_extend_the_window( + self, coord: LocateStub + ) -> None: + """Breaking a window must never lengthen it (the amplification guard). + + ``_note_push_transport_problem`` sets ``_push_cooldown_until`` to + ``monotonic() + 90`` ABSOLUTELY and flags the transport DEGRADED, so + every call restarts the window rather than topping it up. Before this + package that call was unreachable while a window ran -- the suppression + was the first statement of the method -- and the stop button has no + availability guard (``can_stop_sound`` does not exist on the + coordinator, button.py only probes for it). Without this guard a user + who keeps pressing Stop during an outage would restart the window on + every press, and that window also gates Play Sound and manual locate: + two unrelated features stay disabled for as long as the pressing goes + on. The stop itself is still sent -- that is the point of the exception + -- it just may not lengthen the window that let it through. + """ + + def _arm(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_arm) + coord._api_push_ready.return_value = False + window_ends_at = time.monotonic() + 90.0 + coord._push_cooldown_until = window_ends_at + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + outcome = await coord.async_stop_sound("dev-1") + + assert outcome is StopSoundOutcome.FAILED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + assert coord._push_cooldown_until == window_ends_at + + async def test_repeated_broken_through_stops_never_move_the_window_end( + self, coord: LocateStub + ) -> None: + """The amplification chain: repeated presses must not push the end forward. + + All five presses fall inside the one window under test, which is the + situation the guard is for. The claim is constancy of the end within a + window, not termination in general: that already follows from + ``_note_push_transport_problem`` setting the deadline absolutely. + """ + + def _arm(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_arm) + coord._api_push_ready.side_effect = lambda: ( + time.monotonic() >= coord._push_cooldown_until + ) + window_ends_at = time.monotonic() + 90.0 + coord._push_cooldown_until = window_ends_at + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + for _ in range(5): + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + + assert coord.api.async_stop_sound.await_count == 5 + assert coord._push_cooldown_until == window_ends_at + + async def test_a_failing_stop_still_arms_a_window_when_none_is_running( + self, coord: LocateStub + ) -> None: + """The guard is about EXTENDING, not about arming: the normal path is untouched.""" + + coord._api_push_ready.return_value = True + coord._push_cooldown_until = 0.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord._note_push_transport_problem.assert_called_once() + + async def test_a_raised_connection_error_does_not_extend_the_window_either( + self, coord: LocateStub + ) -> None: + """The same guard covers the typed exception handler, not just the outcome. + + ``api`` is a Protocol, so an implementation that lets an aiohttp error + escape reaches the typed handler rather than returning + TRANSPORT_FAILED. That handler arms the cooldown too, and a + broken-through stop can now reach it, so it must not restart a running + window either. + """ + + def _arm(cooldown_s: int = 90) -> None: + coord._push_cooldown_until = time.monotonic() + cooldown_s + + coord._note_push_transport_problem = MagicMock(side_effect=_arm) + coord._api_push_ready.return_value = False + window_ends_at = time.monotonic() + 90.0 + coord._push_cooldown_until = window_ends_at + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.side_effect = ClientConnectionError("boom") + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + assert coord._push_cooldown_until == window_ends_at + + async def test_a_raised_connection_error_arms_a_window_when_none_is_running( + self, coord: LocateStub + ) -> None: + """Counter-case, so the guard above cannot pass by disarming the handler.""" + + coord._api_push_ready.return_value = True + coord._push_cooldown_until = 0.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord.api.async_stop_sound.side_effect = ClientConnectionError("boom") + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord._note_push_transport_problem.assert_called_once() + + async def test_the_exception_changes_the_reported_outcome_class( + self, coord: LocateStub + ) -> None: + """The price of the exception, stated as a test rather than as prose. + + Breaking the window means the stop reaches the transport, so a case that + used to end as SUPPRESSED can now end as FAILED. services.py maps the + two to different exception translation keys + (``stop_sound_suppressed`` vs ``stop_sound_rejected``), so this is + user-visible and must not drift unnoticed. The counter-case in the same + test keeps the old class for a stop that is NOT correlated. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.TRANSPORT_FAILED + + # No key: unchanged, still never sent. + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + # Same window, same transport, but now a proven key. + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.FAILED + coord.api.async_stop_sound.assert_awaited_once_with("dev-1", "uuid-fresh") + + # ---- the boundary: everything below must stay suppressed ---- + + async def test_keyless_stop_stays_suppressed_during_the_cooldown( + self, coord: LocateStub + ) -> None: + """Without a key the stop would be UNCORRELATED, so it buys nothing.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_stop_stays_suppressed_when_push_is_down_without_a_cooldown( + self, coord: LocateStub + ) -> None: + """The exception is bound to the cooldown, not to push readiness at large. + + A genuinely disconnected push transport is not something this stop + inflicted on itself, and sending into it proves nothing. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = 0.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_expired_cooldown_does_not_break_a_push_outage( + self, coord: LocateStub + ) -> None: + """Boundary of the window: a cooldown that has run out grants nothing.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() - 0.01 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_stale_cached_key_stays_suppressed_during_the_cooldown( + self, coord: LocateStub + ) -> None: + """A key older than SOUND_UUID_MAX_AGE_S cannot be the one this cooldown made. + + The cooldown lasts 90 seconds, the key aged past 30 minutes: it belongs + to an older play, and a stop carrying it would report UNCORRELATED. That + is the keyless case with extra steps, so it stays suppressed. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-old" + coord._sound_request_timestamps["dev-1"] = time.time() - 3600.0 + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + async def test_foreign_explicit_key_stays_suppressed_during_the_cooldown( + self, coord: LocateStub + ) -> None: + """An unverifiable key is a claim, not a handle, so it grants no exception. + + Mirrors the rule one layer down: an explicitly passed key only proves + correlation when it IS our own fresh cached key. + + We DO hold a live key for this device here, and that is the point: the + discriminating fact is not "some key exists for dev-1" but "the key + going on the wire is ours". Sending the caller's string would report + CANCELLED for a ring we never addressed, and spend our own handle doing + it. + """ + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-ours-fresh" + + outcome = await coord.async_stop_sound("dev-1", request_uuid="foreign-uuid") + + assert outcome is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + + +class TestCorrelationPredicateIsShared: + """The cooldown gate and the outcome/pop branch must read ONE definition. + + ``_stop_would_be_correlated`` was extracted precisely because two decisions + depend on the same question -- may this stop break a self-inflicted push + cooldown, and may an accepted stop spend the cached key -- and a second, + inline re-derivation at either site is free to drift away from the first. + These two tests bind the sites to the predicate by making the predicate + disagree with the raw cache state: an inline re-derivation would read the + cache and answer the opposite, so it cannot pass. + """ + + async def test_gate_follows_the_predicate_against_the_raw_cache( + self, coord: LocateStub + ) -> None: + """Predicate says no while the cache holds a fresh key of ours.""" + + coord._api_push_ready.return_value = False + coord._push_cooldown_until = time.monotonic() + 90.0 + coord._sound_request_uuids["dev-1"] = "uuid-fresh" + coord._stop_would_be_correlated = MagicMock(return_value=False) + + assert await coord.async_stop_sound("dev-1") is StopSoundOutcome.SUPPRESSED + coord.api.async_stop_sound.assert_not_called() + coord._stop_would_be_correlated.assert_called_once_with("dev-1", None) + + async def test_outcome_and_pop_follow_the_predicate_against_the_raw_cache( + self, coord: LocateStub + ) -> None: + """Predicate says yes while the cached key has aged past the limit.""" + + coord._api_push_ready.return_value = True + coord._sound_request_uuids["dev-1"] = "uuid-old" + coord._sound_request_timestamps["dev-1"] = time.time() - 3600.0 + coord._stop_would_be_correlated = MagicMock(return_value=True) + coord.api.async_stop_sound.return_value = SoundDispatchOutcome.ACCEPTED + + outcome = await coord.async_stop_sound("dev-1") + + assert outcome is StopSoundOutcome.CANCELLED + assert "dev-1" not in coord._sound_request_uuids + + _ = DEFAULT_MIN_POLL_INTERVAL # silence unused-import lint when production no-ops diff --git a/tests/test_coordinator_sound_uuid.py b/tests/test_coordinator_sound_uuid.py index e7fe0704e..3f94e6b22 100644 --- a/tests/test_coordinator_sound_uuid.py +++ b/tests/test_coordinator_sound_uuid.py @@ -7,7 +7,11 @@ import pytest -from custom_components.googlefindmy.const import StopSoundOutcome +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, + StopSoundOutcome, +) from custom_components.googlefindmy.coordinator import GoogleFindMyCoordinator from custom_components.googlefindmy.coordinator.helpers.cache import ( SOUND_UUID_MAX_AGE_S, @@ -27,9 +31,9 @@ async def test_async_play_sound_stores_uuid() -> None: api_calls: list[SimpleNamespace] = [] - async def _async_play_sound(device_id: str) -> tuple[bool, str]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: api_calls.append(SimpleNamespace(device_id=device_id)) - return True, "uuid-1" + return PlaySoundResult(SoundDispatchOutcome.ACCEPTED, "uuid-1") coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -61,9 +65,9 @@ async def test_async_play_sound_skips_store_on_non_accepted_play() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # Non-accepted play (pre-dispatch guard / rejection): no cancel key. - return False, None + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -95,10 +99,10 @@ async def test_non_accepted_play_keeps_existing_cancel_key() -> None: coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # New attempt not accepted (e.g. server 401/403, or pre-dispatch): the # success-only contract yields (False, None) — no cancel key. - return False, None + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -130,10 +134,10 @@ async def test_post_dispatch_ambiguous_play_caches_uuid() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # Post-dispatch ambiguity: not confirmed (ok=False), but a ring may be # active, so the cancel key is returned for caching. - return False, "uuid-postsend" + return PlaySoundResult(SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-postsend") coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -170,9 +174,11 @@ async def test_ambiguous_play_does_not_overwrite_known_cancel_key() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: + async def _async_play_sound(device_id: str) -> PlaySoundResult: # Ambiguous transient 5xx: not accepted (ok=False), fresh UUID present. - return False, "uuid-ambiguous-new" + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-ambiguous-new" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -201,8 +207,10 @@ async def test_ambiguous_play_keeps_fresh_tracked_cancel_key() -> None: coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: - return False, "uuid-ambiguous-new" + async def _async_play_sound(device_id: str) -> PlaySoundResult: + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-ambiguous-new" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -237,8 +245,10 @@ async def test_ambiguous_play_replaces_expired_cancel_key() -> None: ) coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: - return False, "uuid-fresh-ambiguous" + async def _async_play_sound(device_id: str) -> PlaySoundResult: + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-fresh-ambiguous" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -277,9 +287,11 @@ async def test_async_stop_sound_sends_expired_cached_uuid_uncorrelated() -> None api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -317,9 +329,11 @@ async def _save() -> None: api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -348,8 +362,10 @@ async def test_ambiguous_play_keeps_untracked_cancel_key() -> None: coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_play_sound(device_id: str) -> tuple[bool, str | None]: - return False, "uuid-ambiguous-new" + async def _async_play_sound(device_id: str) -> PlaySoundResult: + return PlaySoundResult( + SoundDispatchOutcome.TRANSPORT_FAILED, "uuid-ambiguous-new" + ) coordinator.api = SimpleNamespace(async_play_sound=_async_play_sound) # type: ignore[attr-defined] @@ -389,9 +405,11 @@ async def test_async_stop_sound_uses_cached_uuid() -> None: api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -416,9 +434,11 @@ async def test_async_stop_sound_warns_when_uuid_missing( api_calls: list[tuple[str, str | None]] = [] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: api_calls.append((device_id, request_uuid)) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -456,9 +476,11 @@ async def _save() -> None: coordinator._async_save_sound_uuids = _save # type: ignore[attr-defined] - async def _api_stop(device_id: str, request_uuid: str | None) -> bool: + async def _api_stop( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: sent.append(request_uuid) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_api_stop) # type: ignore[attr-defined] @@ -482,9 +504,11 @@ async def test_blank_request_uuid_without_a_cached_key_is_uncorrelated() -> None coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _api_stop(device_id: str, request_uuid: str | None) -> bool: + async def _api_stop( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: sent.append(request_uuid) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_api_stop) # type: ignore[attr-defined] @@ -522,9 +546,11 @@ async def _save() -> None: sent: list[str | None] = [] - async def _api_stop(device_id: str, request_uuid: str | None) -> bool: + async def _api_stop( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: sent.append(request_uuid) - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_api_stop) # type: ignore[attr-defined] @@ -560,11 +586,13 @@ async def _save() -> None: coordinator._async_save_sound_uuids = _save # type: ignore[attr-defined] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: # A Play completes while the stop is in flight. coordinator._sound_request_uuids[device_id] = "uuid-new" # type: ignore[attr-defined] coordinator._sound_request_timestamps[device_id] = time.time() # type: ignore[attr-defined] - return True + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -592,8 +620,10 @@ async def test_a_rejected_stop_does_not_clear_the_auth_failure_state() -> None: auth_calls: list[dict[str, object]] = [] coordinator._set_auth_state = lambda **kwargs: auth_calls.append(kwargs) # type: ignore[attr-defined] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: - return False + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: + return SoundDispatchOutcome.TRANSPORT_FAILED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] @@ -622,8 +652,10 @@ async def test_an_explicit_key_that_is_ours_but_aged_is_not_called_foreign( coordinator._note_push_transport_problem = lambda: None # type: ignore[attr-defined] coordinator._set_auth_state = lambda **kwargs: None # type: ignore[attr-defined] - async def _async_stop_sound(device_id: str, request_uuid: str | None) -> bool: - return True + async def _async_stop_sound( + device_id: str, request_uuid: str | None + ) -> SoundDispatchOutcome: + return SoundDispatchOutcome.ACCEPTED coordinator.api = SimpleNamespace(async_stop_sound=_async_stop_sound) # type: ignore[attr-defined] diff --git a/tests/test_options_flow_credentials_cache.py b/tests/test_options_flow_credentials_cache.py index 4ae96604c..698b95966 100644 --- a/tests/test_options_flow_credentials_cache.py +++ b/tests/test_options_flow_credentials_cache.py @@ -28,6 +28,7 @@ SUBENTRY_TYPE_SERVICE, SUBENTRY_TYPE_TRACKER, TRACKER_SUBENTRY_KEY, + SoundDispatchOutcome, ) from custom_components.googlefindmy.NovaApi.ExecuteAction.PlaySound import ( start_sound_request as start_module, @@ -492,10 +493,14 @@ async def _fake_stop(scope: str, payload: str, **kwargs: Any) -> str: monkeypatch.setattr(start_module, "async_nova_request", _fake_start) monkeypatch.setattr(stop_module, "async_nova_request", _fake_stop) - success, request_uuid = await api_primary.async_play_sound("device-42") + play = await api_primary.async_play_sound("device-42") + success, request_uuid = play.accepted, play.cancel_key assert success is True assert request_uuid is not None - assert await api_primary.async_stop_sound("device-42", request_uuid) + assert ( + await api_primary.async_stop_sound("device-42", request_uuid) + is SoundDispatchOutcome.ACCEPTED + ) assert start_calls and stop_calls diff --git a/tests/test_services_handler_contracts.py b/tests/test_services_handler_contracts.py index 82bf344a4..dddd0d3f4 100644 --- a/tests/test_services_handler_contracts.py +++ b/tests/test_services_handler_contracts.py @@ -420,10 +420,11 @@ async def test_rejected_stop_does_not_borrow_the_suppressed_advice( ) -> None: """A refused stop must not be reported as a local, transient condition. - SUPPRESSED means "we never sent it, retry shortly". FAILED means the - transport refused it, most often because the sign-in expired. Sharing - one message would give the wrong advice to whichever half is not the - actual cause. + SUPPRESSED means "the push transport is not up yet, retry shortly". + FAILED means the attempt was made and failed, most often because the + sign-in expired. Sharing one message would give the wrong advice to + whichever half is not the actual cause. Note that "was it sent" is not + the criterion -- SoundDispatchOutcome.NOT_SENT is a FAILED as well. """ coord = SimpleNamespace( diff --git a/tests/test_sound_dispatch_contract.py b/tests/test_sound_dispatch_contract.py new file mode 100644 index 000000000..ce6a61cc7 --- /dev/null +++ b/tests/test_sound_dispatch_contract.py @@ -0,0 +1,43 @@ +# tests/test_sound_dispatch_contract.py +"""Pin the API -> coordinator sound contract as a type, not as a bool.""" + +from __future__ import annotations + +import dataclasses + +from custom_components.googlefindmy.const import ( + PlaySoundResult, + SoundDispatchOutcome, +) + +# NOTE: "only TRANSPORT_FAILED may arm the push cooldown" is a property of +# coordinator/locate.py, not of const.py. Asserting it here would only restate +# the enum against itself, so it is pinned where it is observable, in +# tests/test_coordinator_locate_basics.py (see AP-4 of +# PLAN_GFMY_SOUND_FAILURE_CLASSIFICATION), parametrised over every member of +# SoundDispatchOutcome so that future members are covered automatically. + + +def test_result_is_frozen_and_defaults_to_no_cancel_key() -> None: + """A result must not be mutated after the fact, and must not invent a key.""" + + result = PlaySoundResult(SoundDispatchOutcome.REJECTED_AUTH) + + assert result.cancel_key is None + assert result.accepted is False + assert dataclasses.is_dataclass(result) + try: + result.outcome = SoundDispatchOutcome.ACCEPTED # type: ignore[misc] + except dataclasses.FrozenInstanceError: + pass + else: # pragma: no cover - only reached if the dataclass loses frozen=True + raise AssertionError("PlaySoundResult must be frozen") + + +def test_accepted_is_the_only_true_predicate() -> None: + """``accepted`` must not creep into meaning "no error".""" + + for member in SoundDispatchOutcome: + assert PlaySoundResult(member).accepted is ( + member is SoundDispatchOutcome.ACCEPTED + ) diff --git a/tests/test_stop_sound_correlation.py b/tests/test_stop_sound_correlation.py index eace0def4..9faf5c3c7 100644 --- a/tests/test_stop_sound_correlation.py +++ b/tests/test_stop_sound_correlation.py @@ -193,8 +193,9 @@ def test_api_docstrings_declare_no_ring_confirmation() -> None: Drift guard in the style of the FMDN constant checks in tests/test_ble_battery_sensor.py: a boundary that lives only in a design - document gets re-discovered as a bug. Both public sound entry points return - a plain bool, which is exactly the shape that invites "True means it + document gets re-discovered as a bug. Both public sound entry points now + return a classification instead of a bool, but ACCEPTED still only means + "Nova took the submission", which is exactly the shape that invites "so it stopped", so each one carries the disclaimer in its own Returns block. """