fix(sound): classify Play/Stop Sound failures instead of collapsing them into False - #1262
Conversation
`async_play_sound` ran `_set_auth_state(failed=False)` unconditionally, on the failure path as well. `api.async_play_sound` collapses `NovaAuthError` and HTTP 401/403 into the same `(False, None)` it returns for a read timeout, so a play rejected by an expired sign-in did not merely fail: it erased the very signal that the sign-in had expired, and did so on every press for as long as the credentials stayed bad. The sister path already applies the opposite rule and states the reason in a comment (`async_stop_sound`: "No credential proof on this path"). The two paths now agree; the fix is the `else:` that was missing. Scope, so the change is not read as more than it is: a failed play still does not SET the auth-failure state, and still does not trigger reauth. It only stops clearing it. Setting it would raise a Repairs issue and fire an event off a `NovaAuthError` that its own docstring calls possibly transient, which would trade one false signal for another. Tests: `test_failed_play_does_not_clear_auth_state` was written first and verified red against 97edca6 -- exit 1 with `Expected 'mock' to not have been called. Called 1 times. Calls: [call(failed=False)]`, i.e. the defect itself and not a missing file (exit 4) or an unknown test name (exit 5). `test_accepted_play_still_clears_auth_state` guards the positive half and was green before and after. 81 passed across `test_coordinator_locate_basics.py`, `test_coordinator_sound_uuid.py` and `test_stop_sound_correlation.py` (79 before). Known forward breakage, recorded rather than discovered later: the new negative test pins today's `(False, None)` contract of `api.async_play_sound`. Replacing that contract with a typed dispatch outcome will break it by design, and its migration is part of that work.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
api.py already reaches nine distinct exits for a Play Sound command (missing action token, empty submitter reply, HTTP 200, NovaAuthError, HTTP 401/403, other HTTP status, rate limit, network error, unexpected exception), and all of them collapse into a single False at the boundary. The coordinator has no choice but to read that False as "the push transport is broken", so a server saying no, an expired sign-in and a bug of our own each arm the same 90-second cooldown and the same FcmStatus.DEGRADED. This is the failure class StopSoundOutcome already documents one layer up: a bool cannot carry the state space. Add SoundDispatchOutcome (seven values, only TRANSPORT_FAILED justifies a cooldown) and PlaySoundResult (outcome plus cancel key, deliberately two independent facts). Nothing consumes them yet; this commit is additive and changes no behaviour. Also fix tests/helpers/constants.py, which loads const.py through spec_from_file_location/exec_module without registering the module in sys.modules -- the half of the documented importlib recipe that only matters once the module resolves its own annotations at class-creation time. @DataClass does exactly that: 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 returns None and const.py fails to import with AttributeError: 'NoneType' object has no attribute '__dict__' in tests/conftest.py:375, i.e. as a collection error for the whole suite. Without this, const.py cannot host a dataclass at all. Verified: tests/test_sound_dispatch_contract.py fails to collect before the type exists (exit 2, ImportError: cannot import name 'PlaySoundResult') and passes after; mypy --strict and ruff check/format clean; the six sound- related test files stay green (173 passed).
async_play_sound now returns PlaySoundResult and async_stop_sound returns SoundDispatchOutcome, so each exit names who refused instead of collapsing into False: NOT_SENT for a missing local token, REJECTED_AUTH / _SERVER / _RATE_LIMIT for a server that answered and refused, TRANSPORT_FAILED for a network that never gave a usable answer, INTERNAL_ERROR for our own defect or a broken submitter contract. Two of those are new placements rather than renamings. NovaLogicError and NovaProtobufDecodeError were caught by `except NovaError` and would now be reported as transport failures; they are caught before it, as REJECTED_SERVER (the server answered with an error code in a 200 body) and INTERNAL_ERROR (we could not decode what the transport delivered). And async_stop_sound had no `except NovaError` at all -- a network failure after exhausted retries fell through to `except Exception` -- so the branch is added, which is what lets the stop path carry the same table as the play path. Both `except Exception` handlers now log with a traceback: a bug of our own deserves one, and it is no longer indistinguishable from an outage. coordinator/locate.py is adapted minimally and on purpose. A tuple unpack does not work on a dataclass, and a StrEnum is always truthy, so `if not submitted` would have read every rejected stop as a success. It therefore reads play.accepted / play.cancel_key and `stop_outcome is ACCEPTED`, which are exactly the values the old contract carried. Acting on the classification -- above all, arming the push cooldown only for TRANSPORT_FAILED -- is deliberately not part of this commit. The test stubs move with the contract on the same rule: the old False maps to TRANSPORT_FAILED, because today every non-acceptance arms the cooldown, so each existing test keeps asserting the same behaviour as before. docs/PLAY_SOUND_ARCHITECTURE.md defines IRR-CA-SOUND-FAILURE-CLASS, the anchor api.py now cites; leaving the reference dangling is the defect tests/test_stop_sound_correlation.py already guards against. Verified: test_play_sound_classifies_every_exit fails before the change (exit 1, nine failures, AttributeError: 'tuple' object has no attribute 'outcome') and passes after; tests/test_api_basics.py green (152 -> 168); mypy --strict on api.py, const.py and coordinator/locate.py clean; ruff check and ruff format --check clean tree-wide; changed-code coverage measured against the diff: 207 changed lines in api.py and 20 in locate.py, none uncovered. Full suite 6605 tests, 6584 passed, 19 skipped, 2 failed -- both in tests/test_main.py, both failing on the untouched parent commit as well (subprocess tests whose child is SIGKILLed after a 30s timeout in this environment).
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72e4602ff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # part of this one. Until then the two facts are unpacked to exactly | ||
| # the values the old tuple carried, so this changes the type at the | ||
| # boundary and nothing else. | ||
| ok, request_uuid = play.accepted, play.cancel_key |
There was a problem hiding this comment.
When Play Sound returns REJECTED_AUTH, REJECTED_RATE_LIMIT, REJECTED_SERVER, NOT_SENT, or INTERNAL_ERROR, this conversion discards the classification and the subsequent if not ok still calls _note_push_transport_problem(); the Stop Sound path repeats the same collapse. Those non-transport failures therefore continue to set FcmStatus.DEGRADED, arm the 90-second cooldown, and disable sound controls, contradicting the new contract and documentation that only TRANSPORT_FAILED may do so. Branch on the outcome and call the cooldown helper only for TRANSPORT_FAILED.
AGENTS.md reference: AGENTS.md:L627-L629
Useful? React with 👍 / 👎.
`SoundDispatchOutcome` documents that only `TRANSPORT_FAILED` justifies arming the push cooldown (const.py, "This is the ONLY outcome that justifies arming the push cooldown"). The coordinator did not act on it: it unpacked the result back down to `accepted`/`cancel_key` and kept deciding on `if not ok`, so a 401/403, a 429, a missing local action token and a bug of our own each still armed the 90-second cooldown, flipped the integration to `FcmStatus.DEGRADED` and made `can_play_sound()` report the button as unavailable -- an outage the integration inflicted on itself over a network that was working, and one that then also suppressed the user's follow-up Stop. Contract stated, contract not enforced, which is what AGENTS.md Rule §9.DOC forbids. Both sound paths now read `outcome`. The blanket `except Exception` handlers stop arming the cooldown as well: `api.async_play_sound` and `api.async_stop_sound` classify every unexpected exception in band and return `INTERNAL_ERROR` rather than raising, the only code before their `try` is the fully guarded `_get_fcm_token_for_action()`, and the one `await` after the call inside the coordinator's own `try` (`_async_save_sound_uuids`) swallows its own errors. Nothing that reaches those handlers came from the push transport. The two typed handlers for `TimeoutError`/`ClientConnectionError`/`ClientError` keep the cooldown on purpose: `api` is a Protocol, and an aiohttp error from an implementation that does not wrap it really is a transport failure. Red probe before the fix: 12 failures against the unchanged production code, `assert True is False` on `_note_push_transport_problem.called` for `rejected_auth`, `rejected_rate_limit`, `rejected_server`, `not_sent` and `internal_error` on both paths, plus `Expected 'mock' to not have been called` in the two exception tests. The four cases that were already correct (`accepted`, `transport_failed` per path) stayed green, so the red is the defect and not a missing test name. The two case tables carry one row per enum member and are pinned to `set(SoundDispatchOutcome)` by their own guard tests, so a member added later cannot silently inherit whatever the cascade happens to do. Reported by Codex on 72e4602.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A play that reaches the wire and then loses the answer stores a cancel key and arms the 90-second push cooldown in the same breath. Because _api_push_ready() short-circuits on that cooldown, async_stop_sound suppressed the stop the key exists for during the first 90 seconds after the play -- exactly when a user reaches for the Stop button. The readiness gate now passes in one further case: a window is running AND the stop would be correlated, meaning it carries our own cached key and that key is still fresh. Anything that could at best report UNCORRELATED (no key, an aged key, a foreign key supplied by the caller) stays suppressed, so the exception only ever buys an attempt that can actually silence the device. Two things keep it honest: - _stop_would_be_correlated() is the single definition of "correlated" behind both this gate and the decision to spend the cached key. A second inline derivation at either site is the drift the extraction exists to prevent, and two tests pin both sites to the predicate. - _note_stop_transport_problem_without_extending() makes sure the exception cannot feed itself. _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 push the end of the window forward and keep manual locate disabled for as long as the pressing goes on. The failure is still reported in full (the DEGRADED flag is not lost); only a window that was already running is restored instead of restarted. The price is a changed outcome class, not 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. That is pinned by a test rather than left implicit. docs/PLAY_SOUND_ARCHITECTURE.md gains the matching entry and corrects two older passages that would otherwise describe the stop path wrongly.
The repository's ruff (0.14.14 in CI) wraps the lambda body rather than the assignment. Both the pinned and the local formatter accept this shape, so it is not a version-specific workaround.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7045173610
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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 |
There was a problem hiding this comment.
When readiness succeeds but the action token disappears or is invalid by dispatch time, api.async_stop_sound() returns NOT_SENT, yet this blanket non-accepted branch converts it to StopSoundOutcome.FAILED. The service consequently reports stop_sound_rejected, whose contract says the request reached the transport, even though no request left the machine; map NOT_SENT to SUPPRESSED and reserve FAILED for attempted dispatches. This also contradicts the newly added SoundDispatchOutcome and StopSoundOutcome documentation.
AGENTS.md reference: AGENTS.md:L627-L629
Useful? React with 👍 / 👎.
| return _play_result_after_failure( | ||
| err, request_uuid, SoundDispatchOutcome.REJECTED_AUTH | ||
| ) |
There was a problem hiding this comment.
For responses such as HTTP 400 or 404, async_nova_request() raises NovaAuthError (nova_request.py:1750-1755), so this handler now returns REJECTED_AUTH and logs an authentication failure even though SoundDispatchOutcome.REJECTED_AUTH is documented as credentials/401/403 only; the stop handler repeats the same mapping. Inspect err.status here and classify non-401/403 client rejections separately so API consumers and logs do not tell users to diagnose credentials for an invalid request or missing device.
AGENTS.md reference: AGENTS.md:L627-L629
Useful? React with 👍 / 👎.
`NovaAuthError` is raised for every non-retryable 4xx, not only for credential rejections: the comment above the raise in nova_request.py names "403 Forbidden, 404 Not Found" itself, and HTTP_RETRY_ELIGIBLE holds no 4xx besides 408 and 429. A 401 that survives the refresh sequence is raised separately with is_permanent=True, which leaves 403 as the only plain credential rejection reaching these handlers. Both sound handlers read the type alone, so a deleted device produced SoundDispatchOutcome.REJECTED_AUTH and an "Authentication failed" ERROR -- telling a user with a perfectly good sign-in to check their credentials. The neighbouring `except NovaHTTPError` already inspected `err.status`; the new shared helper `_classify_nova_auth_error` gives the auth handler the same rule: permanent first, then 401/403, everything else REJECTED_SERVER. No consumer branches on REJECTED_AUTH, so the log line is the entire user-visible effect of this split. It is asserted, level included: a mutation that keeps the classification and restores the single blanket ERROR kills the new guards while every enum assertion stays green. Second finding, docs only: StopSoundOutcome.FAILED began with "Handed to the transport" while listing the missing action token, which never reaches the wire. Mapping NOT_SENT to SUPPRESSED instead -- as suggested -- would answer a missing token with "try again in a moment", an unbounded retry loop; the `stop_sound_rejected` message already names "a missing token" and points at the log. The docs were corrected instead: the two failure states split by REMEDY, not by distance travelled. That sentence lived in four places (const.py twice, coordinator/locate.py's Returns contract, one test docstring); all four now agree, and a stale "returns False" claim from the bool era went with them. Known gap, stated rather than silently left: the device-list handler, the location handler, coordinator/polling.py and coordinator/locate.py still key off the exception type, where a 404 turns into ConfigEntryAuthFailed or feeds the reauth countdown. Narrowing those changes polling and setup behaviour and needs its own regression test. Recorded in api.py's module header and in custom_components/googlefindmy/AGENTS.md so a green suite is not mistaken for tree-wide coverage.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Draft. AP-1 to AP-3 are done; AP-4 through AP-8 will land on this branch.
Opened early so CI and review have a channel while the rest is built.
Background
Triage of an upstream report (BSkando#215) turned up nothing that
applies to
1.7-- thatTypeErrorwas fixed here long ago. It did, however,prompt a look at what
1.7does when a Play or Stop does fail, and three realdefects fell out of that. All three were proven by a red test against
97edca64before anything was planned.
The common root:
api.async_play_soundandapi.async_stop_soundcatch everyexception and return
(False, None). Auth rejection, HTTP 5xx, read timeout anda local
TypeErrorall arrive at the coordinator as the sameFalse, and thecoordinator has to guess what happened. It currently guesses "push transport
problem", which arms a 90-second cooldown, sets
FcmStatus.DEGRADEDand makescan_play_sound()reportFalsefor the duration.In this PR so far
AP-1 -- the auth-failure state is no longer cleared by a failed play
_set_auth_state(failed=False)ran unconditionally in the play path, so a playrejected by an expired sign-in erased the very signal that the sign-in had
expired.
async_stop_soundhas always applied the opposite rule and says why ina comment; the two paths now agree.
A failed play still does not set the auth-failure state and still does not
trigger reauth -- see the commit body for why that is deliberate and not an
oversight.
Two tests, the negative one verified red first (exit 1,
Expected 'mock' to not have been called. Called 1 times. Calls: [call(failed=False)]). 81 passed acrossthe three affected test files, 79 before.
ruffandmypy --strictclean on thechanged module.
AP-2 -- the dispatch outcome types
SoundDispatchOutcome(seven values, onlyTRANSPORT_FAILEDjustifies acooldown) and
PlaySoundResult(outcome plus cancel key, two deliberatelyindependent facts) live next to the existing
StopSoundOutcomeinconst.py.Additive; nothing reads them in this commit.
One test-infrastructure defect surfaced here and is fixed in the same commit:
tests/helpers/constants.pyloadsconst.pyviaspec_from_file_location/exec_modulewithout registering the module insys.modules. That is half ofthe documented importlib recipe, and it only matters once the module resolves
its own annotations at class-creation time -- which
@dataclassdoes. Withoutthe fix, adding any dataclass to
const.pybreakstests/conftest.pywithAttributeError: 'NoneType' object has no attribute '__dict__', i.e. acollection error for the whole suite.
AP-3 -- every exit names its own cause
async_play_soundreturnsPlaySoundResult,async_stop_soundreturnsSoundDispatchOutcome. Two placements are new rather than renamed:NovaLogicErrorandNovaProtobufDecodeErrorwere caught byexcept NovaErrorand would now be reported as transport failures. They are caughtbefore it, as
REJECTED_SERVERandINTERNAL_ERROR.async_stop_soundhad noexcept NovaErrorat all; a network failure afterexhausted retries fell through to
except Exception. The branch is added, sothe stop path can carry the same table as the play path.
coordinator/locate.pyis adapted minimally and on purpose: a tuple unpack doesnot work on a dataclass, and a
StrEnumis always truthy, soif not submittedwould have read every rejected stop as a success. It reads
play.accepted/play.cancel_keyandstop_outcome is ACCEPTED-- exactly the values the oldcontract carried. Acting on the classification is AP-4, not this commit.
The test stubs move on the same rule: the old
Falsemaps toTRANSPORT_FAILED, because today every non-acceptance arms the cooldown, soeach existing test keeps asserting the same behaviour as before.
Verified red first (exit 1, nine failures,
AttributeError: 'tuple' object has no attribute 'outcome').tests/test_api_basics.py152 -> 168 tests, green.mypy --stricton the three changed modules clean,ruff checkandruff format --checkclean tree-wide. Changed-code coverage measured against thediff: 207 changed lines in
api.py, 20 inlocate.py, none uncovered.AP-4 (commit
dd950f0b): the coordinator reads the classificationReported by Codex on
72e4602f: the conversion back toaccepted/cancel_keythrew the classification away and the following
if not okstill armed thecooldown for every non-acceptance, contradicting
const.py("This is the ONLYoutcome that justifies arming the push cooldown") and therefore Rule §9.DOC.
The contract was stated but not enforced at its only consumer.
Both sound paths now branch on
outcome. The blanketexcept Exceptionhandlers stop arming the cooldown too, because nothing that reaches them came
from the push transport:
api.pyclassifies every unexpected exception in bandand returns
INTERNAL_ERRORinstead of raising, the only code before itstryis the fully guarded
_get_fcm_token_for_action(), and_async_save_sound_uuidsswallows its own errors. The two typed handlers for
TimeoutError/ClientConnectionError/ClientErrorkeep the cooldown, sinceapiis a Protocol and an unwrapped aiohttp error there really is a transportfailure.
Red probe before the fix: 12 failures against the unchanged production code; the
four cases that were already correct stayed green.
Still to come on this branch
fd399eba): a stop is no longer suppressed by the cooldown its own play just armed(False, None)contract, including the one added in AP-1AP-8 is optional and may be dropped.
Review notes
be held hostage to it.
reviewing closely is whether the mapping in
api.pymatches what eachexception really means, since AP-4 will act on it.
occurrences across eleven test files, which is why it is staged rather than
done in one commit.
is the decision table, not the diff shape:
tests/test_coordinator_locate_basics.pycarries one row per
SoundDispatchOutcomemember for each path, and two guardtests pin those tables to
set(SoundDispatchOutcome).AP-5 (
fd399eba)A play that reaches the wire and then loses the answer stores a cancel key and
arms the 90-second push cooldown in the same breath.
_api_push_ready()short-circuits on that cooldown, so
async_stop_soundsuppressed the stop thatkey exists for during the first 90 seconds after the play -- exactly when a user
reaches for the Stop button.
The readiness gate now passes in one further case: a window is running and
the stop would be correlated (our own cached key, still fresh). Anything that
could at best report
UNCORRELATEDstays suppressed.What to review closely:
_stop_would_be_correlated()is the single definition behind two decisionsthat must not drift: may this stop break the window, and may an accepted stop
spend the cached key. Two tests make the predicate disagree with the raw cache
state, so an inline re-derivation at either site cannot pass them.
_note_stop_transport_problem_without_extending()exists because theexception must not feed itself.
_note_push_transport_problem()sets_push_cooldown_untilabsolutely and the stop button has no availabilityguard, so an unguarded call would let repeated presses push the end of the
window forward and keep manual locate disabled meanwhile. The failure is still
reported in full -- the
DEGRADEDflag is not lost -- only a window that wasalready running is restored rather than restarted.
SUPPRESSEDnow reaches the transport and can end as
FAILED, whichservices.pyreportswith a different translation key. There is a test for exactly that.
Known and deliberately not in this commit: the play path arms the same
cooldown without the guard, and
can_play_sound()returns early on a cachedcan_ringcapability before it looks at the window. That predates this change;docs/PLAY_SOUND_ARCHITECTURE.mdrecords it as a follow-up.