fix(auth): classify a Nova refusal by its status, not by the exception type - #1263
Merged
Conversation
NovaAuthError is raised for every non-retryable 4xx: HTTP_RETRY_ELIGIBLE holds no 4xx besides 408 and 429, so 400, 404, 405, 409 and 422 all reach a handler as "auth". A 401 that survived the refresh sequence arrives flagged is_permanent=True, which leaves 403 as the only plain credential rejection a handler sees. Reading the TYPE therefore told a user whose device was removed from the Google account that their sign-in had expired, and the device-list handler did so on the first occurrence, with no threshold in front of it. Give the criterion exactly one home. nova_request.is_credential_rejection answers whether a NovaAuthError really names a credential problem: permanence first, then 401/403, an unreadable status keeps the conservative verdict. api._classify_nova_auth_error becomes its sound-path adapter instead of a second copy of the same test -- a second copy is how the sound handlers and the other four drifted apart in the first place. The device-list handler now takes the exit its 5xx sibling one branch up already takes for a non-credential rejection: UpdateFailed, ApiStatus.ERROR, no reauth reason, no Repairs issue. The 401/403 path is untouched and pinned by a non-regression test. Behaviour note: during a config-flow device probe, a 400/404/422 now maps to the error key "unknown" instead of "invalid_auth", because _map_api_exc_to_error_key classifies on the class name and UpdateFailed carries no "auth". That is intended -- the sign-in is intact -- and a characterisation test pins the pair so the shift is recorded rather than silent. A dedicated key would need strings.json plus the full translation sync and is out of scope here.
…efusal
The location handler re-raised every NovaAuthError into the coordinator,
which fed the transient-auth counter. Three poll cycles of a device that
had been removed from the Google account therefore produced a re-auth
prompt, and every cycle logged "Transient authentication error" for a
device whose sign-in was never in question.
Branch on the status via nova_request.is_credential_rejection and take the
exit the 5xx sibling below already takes: log the client rejection with its
status at WARNING and return {}, so the poll cycle skips this device and
keeps going. The 401/403 path and the is_permanent path are untouched, both
pinned by non-regression tests -- including a permanent 404, where the
ordering of the two checks is easiest to get wrong.
This also dries up the only production feeder of the coordinator's
NovaAuthError branches: both coordinator/polling.py and coordinator/locate.py
receive this class exclusively from async_get_device_location.
The poll cycle raised _consecutive_transient_auth_failures for every
NovaAuthError, so a device permanently missing from the account escalated to
a re-authentication prompt after exactly three cycles while the sign-in was
intact throughout.
Branch on the status first and treat a non-credential rejection the way the
StaleOwnerKeyError branch below treats its own case: warn with the status,
record it via note_error so it stays in the diagnostics, mark the cycle
failed and continue with the next device. The counter is touched in NEITHER
direction -- a 404 says nothing about the credentials, and a reset would make
a real 401/404/401 sequence unescalatable. A regression test drives exactly
that sequence.
Reachability, stated so the branch is not mistaken for dead code: after the
api.py narrowing this path is only reachable through an API double, because
async_get_device_location returns {} for such a status. It stays because
polling.py owns the counter, so the rule has to hold locally here; otherwise
the next feeder brings the cascade back. The comment in the branch says so.
…ocate
A manual locate that hit a NovaAuthError called _set_auth_state(failed=True)
regardless of the status. That creates a Repairs issue, fires
EVENT_AUTH_ERROR and turns the auth diagnostic sensor on -- so a device
removed from the Google account showed the user a sign-in problem that did
not exist.
Branch on the status and take the exit the NovaHTTPError sibling below
already takes for a 5xx: warn with the status, record via note_error, return
{}. The auth state is left alone. The 401/403 path is unchanged and now has
a characterisation test, because this handler had no coverage at all before
(git grep for its log wording found nothing under tests/).
…assthrough This passthrough does not classify -- api.py decides between ConfigEntryAuthFailed and an ordinary empty result, and the re-raise is unchanged. It did claim a classification, though: NovaAuthError covers every non-retryable 4xx, so a device removed from the account produced "Authentication error while requesting location" at WARNING, immediately above the line api.py logs about the same event. Two contradictory sentences, the freshly corrected one second. Both records are branched, the WARNING and the DEBUG one. They carried the same wording, so naming only one would have left the false claim standing in the other channel. R6 is preserved: the device name stays at DEBUG. The existing 401 test gains a wording assertion. Without it that test pins only the redaction, and a mutation that sends every status down the client-error branch would have survived unnoticed (git grep for the phrase under tests/ returned nothing).
Eight places claimed a gap that is closed, or stated the criterion as the
exception type. They are corrected, not shortened (Rule §9.DOC):
- api.py module header: the "Known gap" paragraph is replaced by what every
handler now does, plus what stays open.
- api.py async_get_device_location docstring: the ambiguous "NovaAuthError or
NovaHTTPError with status 401/403" is made unambiguous and gains the {} exit.
- custom_components/googlefindmy/AGENTS.md: the rule sentence and the sentence
about a green suite stay verbatim; only the "Extent today" paragraph moves.
- nova_request.py: the NovaAuthError class docstring stops saying "typically
401 or 403", the comment above the raise says why the type name is wider
than its content, and the Raises: block points at the predicate.
- start_sound_request.py / stop_sound_request.py: the identical "Transient
auth error" comment lived in both files; both are corrected.
Two claims are deliberately narrowed after an independent review of the full
diff, because they were true of the branch and false of the path. Since
api.py returns {} for a non-credential rejection, the poll cycle and the
manual locate now take their SUCCESS path, which resets the transient-auth
counter and clears the auth state before it checks whether the result was
empty. That reset is not new -- every 5xx and 429 already reaches it -- but it
is newly reachable for a client rejection, and it can mask a genuine 401 on
another tracker in the same cycle. The branch comments, the AGENTS.md
paragraph and the two test docstrings now say so instead of implying the
opposite. Fixing it means deciding what an empty result may prove about
credentials; that is a behaviour change of its own and is tracked separately.
Also corrected: "three of which have a broad handler below them" was wrong.
Measured over the AST, all eight try blocks that catch NovaAuthError carry a
broad except Exception in the same block, so the follow-up that gives the
non-credential case its own class is riskier than the note claimed.
Round two of the terminal diff review found that round one replaced one
wrong number with another and left three statements standing that this
branch itself falsified. All four are corrected against a measurement, not
against an estimate:
- "all eight try blocks carry a broad except Exception" was wrong twice
over. Measured over the AST: ten try blocks catch NovaAuthError. Eight
carry a broad handler and would swallow a new exception class; the two
sound-request handlers catch it in a tuple with NO broad handler, so there
a new class would propagate uncaught instead. Two opposite failure modes,
which makes the follow-up riskier than either number suggested.
- "can mask a genuine 401 in the same cycle" understated it. What a client
rejection adds to the pre-existing empty-result reset is PERMANENCE: a 5xx
clears up, a deleted device does not, so one deleted tracker resets the
counter every cycle and a genuinely expired sign-in on another tracker
never reaches the threshold at all. Before this branch that 404 raised the
counter itself, so the escalation did happen, for the wrong reason. Stated
plainly now, including the reason it is not fixed here: the fix has a far
wider blast radius, because every healthy idle BLE poll takes the same path.
- const.py quoted the NovaAuthError docstring verbatim ("4xx client errors").
The previous commit reworded that docstring and left the quote dangling.
- location_request.py's second handler still said transient errors are
tracked by the coordinator. True for a credential rejection only; a
non-credential 4xx never reaches a counter now.
- The count of predicate readers is now measured (six call sites in four
files, serving seven handlers) instead of asserted.
Owner
Author
|
@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: 92be8877b9
ℹ️ 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".
| device_id, | ||
| _short_err(err), | ||
| ) | ||
| return {} |
There was a problem hiding this comment.
When a stale or deleted tracker produces a non-credential 4xx such as 404, returning {} erases the reason the result is empty. Both production callers treat this normal return as authenticated success before checking emptiness: polling clears _consecutive_transient_auth_failures and the account auth state, while manual locate also clears the auth state. A persistent rejected tracker can therefore repeatedly erase a genuine 401/403 from another tracker and prevent the reauthentication prompt; propagate a distinguishable client-rejection outcome or avoid resetting auth state without positive authentication evidence.
AGENTS.md reference: custom_components/googlefindmy/AGENTS.md:L120-L130
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…sult Review of PR #1263 surfaced two findings on the location handler. Both are fixed here. Finding 1 (P1, correctness). Returning `{}` for a non-credential 4xx traded one defect for another. Both production callers read a NON-RAISING return as positive proof that the credentials work: coordinator/polling.py clears the auth state and resets the transient-auth counter, and coordinator/locate.py clears the auth state, each BEFORE it checks whether the result is empty. A tracker permanently rejected by the server would therefore have wiped a real 401 from another tracker in every poll cycle, and the re-auth prompt this whole change exists to postpone would never have appeared at all -- a worse outcome than the defect being fixed. api.async_get_device_location now passes the error through. The branches added to polling.py and locate.py in this same PR were written for exactly this status and skip the counter deliberately, so they are now the path a rejected device really takes rather than statements about an unreachable one. The log record drops to DEBUG because both callers already emit a WARNING naming the device; it stays for the sync wrapper, which has no handler behind it. The success-path reset itself is NOT changed. Every 5xx, every 429 and every idle BLE tag still reaches it, which is wrong on its own terms and is tracked as a finding of its own with its own approval gate. test_an_empty_return_still_clears_the_counter characterises that reset so it cannot drift silently, and test_a_non_credential_4xx_location_is_passed_through pins the seam that keeps a rejection out of it. Finding 2 (coverage). The false side of the `if last_exception is None` guard in the poll loop was untested. A credential failure on one tracker must stay the reported cause of the cycle even when a rejected tracker follows it, otherwise the surviving error names the harmless device. Also sharpened: with both exits now raising NovaAuthError, the 403 counterpart test no longer discriminated on `pytest.raises` alone -- it would have passed even if every status took the client-rejection branch. It asserts on the log record instead. Swept for copies of the claim that api.py returns `{}` for such a status: seven sites across api.py, polling.py, locate.py, location_request.py, AGENTS.md and two test modules, all corrected in this commit.
An independent review round found two claims in this branch that no test and no reader could tell apart from a fact. The first was self-inflicted. 3cac2ea turned the location handler from returning {} into re-raising, and its own commit message says the claim was swept across seven sites. It missed the bullet directly above the code it changed: api.py still promised "returns `{}` like a 5xx does". The sweep pattern looked for the phrase, and that copy spells it with backticks. A maintainer reading that line would have restored the very defect the commit removed. Fixed there, in the Returns: section, and in the Protocol one level up, which carried the same promise before this branch and now omits two exception classes instead of one. The method also gained the Raises: section AGENTS.md requires for a public function. The second was a test that named itself after a change it did not exercise. test_a_rejected_device_probe_no_longer_reads_as_a_bad_sign_in only called _map_api_exc_to_error_key with two hand-built exceptions; revert the device-list branch and it stayed green. Its docstring admitted this, its name did not. It is now named for what it does, and the real path is pinned in test_api_basics.py: take the exception FROM the narrowed handler, then map it. Mutation-checked - disable the branch and the new row fails with invalid_auth where it expects unknown. Three smaller things in the same pass. The AGENTS.md paragraph counts six call sites, ten try blocks and eight broad handlers, calls itself "the only thing standing in its way", and nothing enforced a single one of those numbers; TestTheDocumentedExtentStaysTrue now derives them from the AST, the way tests/AGENTS.md already prescribes for shared tuples. The Example doctest on is_credential_rejection never ran, because pyproject.toml passes no --doctest-modules, so it read like a verified assurance while rotting silently; it is prose now, naming the test that does run. And a seeded _last_transient_auth_error held an exception object where the declared type is str | None, asserted only as "not None" - it now seeds a str and asserts equality, which is what catches an overwrite. One review finding is rejected with a measurement rather than accepted. The new locate.py branch was flagged for reusing where="async_locate_device" instead of a dedicated value like polling.py's "poll_client_error". All ten note_error calls in locate.py use that one value; polling.py uses four distinct poll_* values. Each file has its own consistent convention and this branch follows the one it is in. A special value in locate.py would be the break, not the repair. The two cases also are distinguishable: note_error stores f"{prefix}: {exc}", so the status code is in the record. Measured: ruff check, ruff format --check and mypy --strict clean tree-wide; four mutations each kill the predicted guard; no executable production line changed, only docstrings.
…t see The CI run on 3cac2ea failed five checks with a single root cause: ruff reported PLW0108 (unnecessary-lambda) on the recorder installed by test_a_client_error_does_not_overwrite_an_earlier_failure. `lambda exc: recorded.append(exc)` is exactly `recorded.append`, so the lambda adds a frame and nothing else. Four of the five red checks were the same finding seen twice: the Lint (ruff) job, and test_platinum_compliance.py:: test_ruff_linting_compliance, which shells out to `ruff check .` and so turns a lint finding into a test failure on all three Python matrix legs. Why the local gate stayed green: PLW0108 is preview-gated in ruff 0.14.14, the version in the local venv, which answers a bare `--select PLW0108` with "has no effect because preview is not enabled". CI installs via `poetry install --with dev,test`, so the version comes from poetry.lock (ruff 0.16.3), not from the `>=0.14.1` floor in pyproject.toml, and there the rule is stable. The repo already knows this failure mode -- see the PLR0917 comment in [tool.ruff.lint.per-file-ignores]. Verified against ruff 0.16.3 in a throwaway venv, so the pinned test environment stays untouched: on the parent commit `ruff check .` reports exactly this one error tree-wide, and with the fix it reports none; `ruff format --check .` is clean either way. The behaviour the test pins is unchanged: removing the `if last_exception is None:` guard in polling.py still makes it fail.
jleinenbach
force-pushed
the
fix/nova-auth-status-classification
branch
from
August 26, 2026 22:53
c88f4a7 to
6c25a8f
Compare
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c25a8fbbb
ℹ️ 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".
Comment on lines
+1279
to
+1283
| - A credential rejection raises `ConfigEntryAuthFailed` so the | ||
| coordinator can start re-auth. That means a `NovaHTTPError` with | ||
| status 401/403, or a `NovaAuthError` for which | ||
| `nova_request.is_credential_rejection` holds (permanent, 401, 403, | ||
| or no readable status). |
There was a problem hiding this comment.
For a plain credential rejection such as NovaAuthError(403, ..., is_permanent=False), is_credential_rejection returns true, but the handler below only converts permanent errors and otherwise re-raises the original NovaAuthError so the polling coordinator can apply its retry threshold. The new protocol and implementation docstrings instead promise ConfigEntryAuthFailed for every credential rejection, so callers following this public contract may catch the wrong exception and allow a transient 403 to escape unexpectedly; document the permanent versus thresholded paths accurately.
AGENTS.md reference: AGENTS.md:L628-L628
Useful? React with 👍 / 👎.
…contract An external review found that the docstrings added by this branch promise more than the handler delivers. They said a credential rejection raises `ConfigEntryAuthFailed`. The handler converts only a PERMANENT auth failure (and an HTTP 401/403 `NovaHTTPError`); a plain non-permanent credential rejection -- in practice a 403, since a 401 that survives the refresh arrives flagged permanent -- is re-raised as `NovaAuthError` on purpose, so `polling.py` can count it against `_MAX_TRANSIENT_AUTH_FAILURES` instead of prompting the user on the first occurrence. That asymmetry to the device-list handler (which does convert every credential rejection, having no counter behind it) was real in the code and absent from the text. A reader who trusted the text would either catch the wrong exception or "fix" the code to match the docstring. Measured, so the finding is not overstated: both production callers already catch `NovaAuthError` AND `ConfigEntryAuthFailed` (`coordinator/polling.py` 2076/2165, `coordinator/locate.py` 561/551), so no caller misbehaves today. The damage was prospective, not present. Corrected in all four places the claim lived, found by sweeping the class and not just the line the reviewer quoted: - the `GoogleFindMyAPIProtocol.async_get_device_location` docstring - the implementation's "Auth mapping (5.1-D)" block - the implementation's `Raises:` section - the module docstring's 401/403 bullet (pre-existing, imprecise in the same way; low risk, low effort, corrected rather than left as a trap) `custom_components/googlefindmy/AGENTS.md` gains the same asymmetry, because that file is what the next agent reads before touching this handler. `tests/test_api_basics.py::test_a_credential_rejection_location_takes_the_auth_exit` already pinned the behaviour; its docstring now names the contract it guards, so the coupling is visible from the test side too. Verified by mutation: forcing `if err.is_permanent:` to `if True:` in the location handler (i.e. making the code obey the OLD, wrong docstring) turns that test red. No executable line changed. Gates, tree-wide: `ruff check .`, `ruff format --check .` (also re-run with the exact `poetry.lock` pin 0.16.3, per the lesson from the `PLW0108` failure earlier on this branch), `mypy --strict` (123 source files), and the six affected suites: all green. Refs: AGENTS.md rule 9.DOC (correct and augment, do not shorten).
jleinenbach
marked this pull request as ready for review
August 27, 2026 08:38
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9df5af7b4
ℹ️ 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".
| where="poll_client_error", | ||
| device=dev_name, | ||
| ) | ||
| cycle_failed = True |
There was a problem hiding this comment.
When one stale or deleted tracker returns a persistent non-credential 4xx while sibling trackers still update successfully, setting cycle_failed here and retaining the exception below causes the finally block to call async_set_update_error at line 2306. GoogleFindMyEntity.available follows the coordinator's last_update_success (entity.py lines 343–358), so this per-device rejection makes every tracker entity unavailable on each poll until the rejected tracker disappears from the cached device list. Treat this branch as a recorded per-device skip, or fail the cycle only when no device succeeds, rather than replacing the erroneous reauthentication with an account-wide availability outage.
Useful? React with 👍 / 👎.
… cycle The client-error branch added by this PR recorded the rejection as the cycle's `last_exception`. In the cycle's `finally` block that is the sole driver of `async_set_update_error`, and `GoogleFindMyEntity.available` follows the coordinator's `last_update_success` (`entity.py`), so ONE rejected tracker marked EVERY tracker entity unavailable. Unlike a 5xx, a device deleted from the Google account never recovers, so the outage repeated on every single poll for as long as that tracker stayed in the cached device list. That trades a spurious re-authentication prompt for a permanent availability outage, which is not a fix. `cycle_failed` and `last_exception` drive two different things and are now treated as such: `cycle_failed` stays (it only writes the `last_poll_result` diagnostic attribute that `binary_sensor.py` exposes, and the cycle really did fail to poll every device), while `last_exception` is left to the failures that are about the account. That is the same shape the neighbouring `OwnerKeyLookupTransientError` branch already uses for an ordinary per-device skip. Side effect worth naming, found by the new order-sensitive test: while the client branch held the `last_exception` slot, a rejected tracker polled BEFORE a genuinely expired one made the coordinator report the harmless "HTTP 404 gone" and hide the 401 entirely. Residual gap, documented rather than glossed over: if EVERY device is rejected the cycle now reports no error at all. The window is narrow -- an account-wide rejection already fails `async_get_basic_device_list` with `UpdateFailed` one layer up -- and it is not silent (`last_poll_result` reads "failed", `note_error` records it, every rejected device gets its own WARNING). Three new tests pin the behaviour, each verified by a mutation that kills exactly it: - `test_a_rejected_device_does_not_make_every_tracker_unavailable` (re-adding the `last_exception` assignment kills it) - `test_a_rejected_device_still_marks_the_poll_result_failed` (dropping `cycle_failed` kills it) - `test_a_rejected_device_does_not_hide_a_later_credential_failure` (re-adding the `last_exception` assignment kills it) `custom_components/googlefindmy/AGENTS.md` records the reasoning and the residual gap. No new executable production lines: two were removed, the rest of the diff is prose. Reported by Codex on commit c9df5af.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
Follow-up to 9f53b02, which stopped a per-device client rejection from failing the coordinator update. That commit left a gap and excused it with a claim that does not hold. Both are addressed here. The gap: with `last_exception` no longer set in the client branch, a cycle in which EVERY device was rejected reported no error at all. `last_poll_result` read "failed", but `last_update_success` stayed True, so the integration looked healthy while delivering nothing. On a single-tracker install that is the ordinary case, not a corner one. The claim, now retracted: 9f53b02 argued the window was narrow because "an account-wide rejection already fails `async_get_basic_device_list` with `UpdateFailed` one layer up". Measured, that is wrong twice over. `nbe_list_devices` is a different RPC from the per-device location request -- nothing couples a 404 on one tracker's location to the device list -- and `DEVICE_LIST_POLL_INTERVAL` (300s) means most cycles take the `if use_cached_list:` path (polling.py) and never call it. The sentence is removed from the code comment and from AGENTS.md, and AGENTS.md now says explicitly that it was written once and was wrong. The fix follows the shape the file already uses for the decrypt verdict (`_finalize_cycle_decrypt_state`): whether a rejection is per-device or account-wide is only knowable once every device has been tried, so the verdict is deferred to after the loop. `cycle_rejected_devices` counts the rejections; if it equals `len(devices)` there is no sibling success left to refute them and the cycle surfaces an `UpdateFailed`. Two further corrections to 9f53b02's prose, both measured: - "Same shape as the OwnerKeyLookupTransientError branch" was false: that branch sets neither `cycle_failed` nor `last_exception`. The client branch is the only one in the loop that sets one without the other, and the comment now says so instead of inventing a precedent. - The AGENTS.md heading said "without failing the cycle"; the branch does fail the cycle (`cycle_failed = True`), it does not fail the coordinator UPDATE. Two tests were hardened after an independent review found them able to pass vacuously: - `test_a_rejected_device_does_not_make_every_tracker_unavailable` was a bare negative assertion; it now also pins that both devices were requested and that the client branch ran. - `test_a_client_error_does_not_overwrite_an_earlier_failure` passed even with the client branch disabled outright (the 404 would fall through to the transient path, whose own guard preserves the 401). Verified: with `if not is_credential_rejection(...)` forced to False it now fails. New test `test_a_cycle_where_every_device_is_rejected_still_reports_an_error` was red before the fix (`assert []`). Three mutations each kill exactly the predicted guard: removing the post-loop verdict, weakening `==` to `<=` (which reproduces the original finding), and never incrementing the counter. Changed production lines: 11, none in the coverage missing list.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fb0ec261a
ℹ️ 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".
| # list without calling it at all. | ||
| if ( | ||
| cycle_rejected_devices | ||
| and cycle_rejected_devices == len(devices) |
There was a problem hiding this comment.
When one tracker is rejected with a non-credential 4xx and every sibling returns {} because of a 5xx, 429, or network failure, cycle_rejected_devices is nonzero but smaller than len(devices), while none of those empty-result paths populate last_exception. The finally block consequently skips async_set_update_error and leaves last_update_success true even though no request succeeded; before this change, the 4xx at least made that mixed-failure cycle fail. Track positive request success or preserve transient failure outcomes instead of treating any non-rejected device as the sibling success required by this equality check.
AGENTS.md reference: custom_components/googlefindmy/AGENTS.md:L159-L162
Useful? React with 👍 / 👎.
…t does not Codex flagged that a cycle with one rejected tracker and siblings that came back empty from a 5xx reports coordinator success, and asked for positive request-success tracking instead of the `cycle_rejected_devices == len(devices)` equality. The mechanism is confirmed. The remedy is not available at this layer, and measurement says the framing is off: - `_any_device_got_data` is the only positive marker the loop has, and it is set solely after a location was normalised, fused and committed. Gating on it would fail the very common "one deleted tracker plus one idle BLE tag" account on every poll -- re-creating the account-wide availability outage that 9f53b02 removed one review round earlier. - "returned without raising" does not separate the named case either: `api.async_get_device_location` returns `{}` for a 5xx, a 429, a protobuf decode failure and a Nova logic error just as it does for a healthy idle tag. - Measured on the reference case: a cycle in which EVERY device returns empty, with not one rejection involved, reports `success` today and leaves every entity available with its cached position. So the class is pre-existing and independent of the rejection branch. Making only the rejection branch stricter would tie the error signal to whether some unrelated tracker happens to have been deleted -- a coincidence, not a contract. What was actually wrong is the prose. The comment said "no sibling success left to refute the rejections", which invites exactly the inference Codex made: that a non-rejected sibling is evidence something worked. The neighbouring decrypt verdict shows the difference -- it gates on `cycle_had_successful_decrypt`, a positive proof the request path has no counterpart for. The guard is now described as what it tests: every device was refused, which is account-wide on the rejections' own terms, and nothing more. Also recorded: the mixed cycle is not silent everywhere. The rejection still sets `cycle_failed`, so `last_poll_result` reports `failed` and the diagnostic binary sensor shows it; only entity availability is left alone. Both states are pinned so the follow-up flips them deliberately rather than silently: `test_a_cycle_of_only_empty_results_reports_success` holds the reference case, `test_a_mixed_cycle_of_rejection_and_empty_siblings_stays_silent` holds the mixed one. Mutating the empty path to record an exception kills the first; weakening the equality to a bare truthiness check kills the second. `TestTheDocumentedRejectionGuardStaysTrue` enforces the paragraph's own count against the names it lists, AST-derived. That number had to be corrected from four to six in this commit by hand, which is precisely the drift the rule in `tests/AGENTS.md` exists to catch. No executable production line changes.
Owner
Author
|
@codex review - did the last commit fix the previous finding "Handle mixed no-data failures before reporting coordinator success"? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9899a2452c
ℹ️ 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".
Comment on lines
+2316
to
+2317
| and cycle_rejected_devices == len(devices) | ||
| and last_exception is None |
There was a problem hiding this comment.
When one device is rejected and every sibling returns {} because of a 5xx, 429, network, decode, or Nova logic failure, this equality remains false and none of the empty-result paths sets last_exception; the finally block therefore calls async_set_updated_data without async_set_update_error, leaving last_update_success true despite no successful request. Fresh evidence in this commit is test_a_mixed_cycle_of_rejection_and_empty_siblings_stays_silent, which explicitly asserts that this failure is not surfaced, so the previous finding remains unfixed; preserve distinguishable transient outcomes or track positive request success before suppressing the rejection.
AGENTS.md reference: custom_components/googlefindmy/AGENTS.md:L156-L164
Useful? React with 👍 / 👎.
…that fixes it An external reviewer reported the same finding twice: a cycle in which one device is rejected with a non-credential 4xx and every sibling comes back empty from a 5xx reports success, because none of the empty-result paths sets `last_exception`. The finding is correct. What was wrong here is not the guard but the paperwork around it. Measured, and it is why the guard cannot simply be tightened: `api.async_get_device_location` has eight non-raising exits that all return the same empty dict (api.py 1401, 1499, 1508, 1518, 1529, 1538, 1567, 1574, 1583), and exit 1401 is itself ambiguous because the transport layer collapses nine outcomes to `[]` (location_request.py 743, 751, 783, 790, 820, 825, 843, 879, 910). Line 843 is the HEALTHY idle BLE tag with no reporter in range; line 790 is a 5xx. For the poll loop the two are bit-identical: there is no status field, no sentinel, no counter on the API object, and `is_fcm_connected` describes the push transport, not this request. Any guard at this layer that treats "no device returned data" as a failure therefore takes every healthy BLE-only account offline on every cycle -- pinned by `test_known_gap_a_cycle_of_only_empty_results_reports_success`. So the gap is real and its fix is a contract change one layer down, not a predicate change here. Three things are corrected: 1. The three places that pointed at `PLAN_GFMY_AUTH_RESET_POSITIVE_PROOF` for that fix pointed at the wrong plan. That plan deliberately chooses the option that does NOT make the empty result distinguishable; it moves the auth reset instead. They now name `PLAN_GFMY_EMPTY_RESULT_DISTINGUISHABLE`, and the unnamed "tracked as a separate finding" in api.py names it too. A reference is evidence only if what it points at contains the thing. 2. The regression is named where it happens. Before this branch the mixed cycle DID surface, because the 4xx took the transient-auth branch and set `last_exception` there -- the same branch that fed the counter behind the false sign-in prompt. That signal was a side effect of the misclassification, not a contract, and it cannot be kept without keeping the defect. 3. The two tests that pin the gap are renamed to say so: `test_known_gap_a_cycle_of_only_empty_results_reports_success` and `test_known_gap_a_mixed_cycle_of_rejection_and_empty_siblings_stays_silent`. Their old names read like a contract, and the reviewer cited one of them as evidence that the behaviour was intended. No executable production line changes: `git diff` on polling.py and api.py is comments only. Gates: `ruff check .`, `ruff format --check .` (497 files) and `mypy --strict` (123 files) tree-wide green; the three affected suites exit 0. Two mutations kill the contract guard that pins these names (`test_every_named_test_exists` when a name is reverted in AGENTS.md, `test_the_stated_number_matches_the_names_it_lists` when the count is falsified), tree restored byte-identically after each.
Owner
Author
|
@codex review - did the last commit fix the previous finding "Handle mixed no-data failures before reporting coordinator success"? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c7eab2a7f
ℹ️ 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".
Comment on lines
+2328
to
+2330
| cycle_rejected_devices | ||
| and cycle_rejected_devices == len(devices) | ||
| and last_exception is None |
There was a problem hiding this comment.
When one tracker receives a non-credential 4xx while every sibling returns {} because of a 5xx, 429, network, decode, or Nova logic failure, this equality remains false and no path records last_exception; the finally block consequently calls async_set_updated_data without async_set_update_error, leaving last_update_success true even though no request succeeded. Fresh evidence in this commit is the renamed test_known_gap_a_mixed_cycle_of_rejection_and_empty_siblings_stays_silent plus the preceding comment explicitly identifying this behavior as a regression, so the previous finding is not fixed; preserve distinguishable transient outcomes or track positive request success before suppressing the rejection.
AGENTS.md reference: custom_components/googlefindmy/AGENTS.md:L156-L168
Useful? React with 👍 / 👎.
The reviewer asked twice for the mixed rejection-plus-empty cycle to
surface and proposed that the guard track positive request success.
Measured, that cannot be done at this layer, and the prose here answered
with a forecast about BLE-only accounts instead of a fact.
The fact: `_any_device_got_data` is the only positive proof the poll loop
holds, and it records that a device COMMITTED data, not that its request
reached the server. Gating the all-rejected guard on it turns
`test_a_rejected_device_does_not_make_every_tracker_unavailable` red,
because that test's sibling returns exactly `{}`. One rejection plus one
empty sibling has to stay silent for that test and surface for the mixed
one, from the same observable state. That is an ambiguity, not a
judgement call, and only the layer that produced the empty result can
resolve it.
Records where that is, so the follow-up does not start by re-deriving it:
`location_request.py` logs `Location request accepted` once the RPC is
through; every `return []` before that point is a failure and every empty
result after it is legitimate. It catches the 5xx itself, so `api.py`'s
`NovaHTTPError` handler never sees that case and an intervention confined
to `api.py` would be inert.
The replaced claim was wrong, not merely vague: the guard is conjunctive
with a rejection, so an account without one is never touched by a
stricter form of it. Swept the tree for copies of the claim; it existed
once, and the sibling docstring that called the same `{}` "polled fine"
is corrected with it.
No executable production line changes.
Owner
Author
|
@codex address that feedback: The fact: Records where that is, so the follow-up does not start by re-deriving it: The replaced claim was wrong, not merely vague: the guard is conjunctive No executable production line changes. |
Summary
Testing
|
An adversarial read of the previous commit could not refute its core claim, but found six errors in the prose that carries it. Four are factual and are corrected here; the argument survives them, which is exactly why they had to go. 1. "the only positive proof the poll loop holds" contradicts its own neighbour twelve hundred lines up, which calls `cycle_had_successful_decrypt` "positive proof the shared key works". Correct: the only positive marker on the REQUEST path. Both are False for either empty result, so the conclusion is unchanged. 2. "every empty result AFTER it is legitimate" is false. The unexpected-device branch logs a WARNING and returns empty, after the accept line. 3. The outer surfacing handler was listed among the returns "BEFORE that point". Its `return []` sits after the accept log; the exceptions it catches come from either side. The split is therefore not positional, and a follow-up needs a flag set at the log line, not a line number. 4. `location_request` catches the 5xx, the 429, the protobuf decode failure AND the Nova logic error, so four `api.py` handlers never run on the locate path. The empty dict comes from the no-location fallthrough. The outcome the older paragraph describes is right; the mechanism it names is not. Also restores what the previous commit threw out with the wrong quantifier: an account with one deleted tracker and otherwise idle BLE tags WOULD go unavailable on every cycle under a stricter gate. "Every healthy BLE-only account" was wrong, that one shape is not, and it is the concrete price of tightening here. Deleting a claim because its quantifier is too wide loses the part that was true. Names the near-duplication the review turned up: the availability test and the known-gap test share stub and assertions and differ only in a display name. That is the point rather than an oversight, and the docstrings now say so. No executable production line changes.
… not rebuilding it An external review of this PR proposed two constraints on the follow-up tracked as `PLAN_GFMY_EMPTY_RESULT_DISTINGUISHABLE`: the accepted-versus- failed outcome has to cross the `location_request.py` boundary in a distinguishable form (a typed result, for example), and it must not be reconstructed downstream from the empty collection. Both hold, and neither was stated in the contract -- only in the plan, which a reader of `AGENTS.md` is not obliged to open. The review attributed the loss of the distinction to `api.py`. Measured, that is wrong and the paragraph directly above already says so: `location_request.py` catches the 5xx, the 429, the protobuf decode failure and the Nova logic error itself (`location_request.py:779/784/791/821`, all before the `Location request accepted` log at `:828`), so `api.py`'s handlers for those four never run on the locate path. Aiming the follow-up at the file where the empty dict is built would put the fix behind the point where the evidence is already gone. The wrong attribution is named here because it is the plausible reading, not because the reviewer was careless. Prose only: no executable production line changes, and no count or test name in the touched paragraphs, so `TestTheDocumentedExtentStaysTrue` is not affected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
NovaApi/nova_request.pyraisesNovaAuthErrorfor every non-retryable 4xx, not only for credential rejections:HTTP_RETRY_ELIGIBLEholds no 4xx besides 408 and 429, so 400, 404, 405, 409 and 422 all arrive at a handler as "auth". A 401 that survived the refresh sequence is raised separately withis_permanent=True, which leaves 403 as the only plain credential rejection a handler ever sees.Four handlers read the type instead of the status, so a device removed from the Google account was reported as an expired sign-in:
api.pydevice listConfigEntryAuthFailedon the first occurrence, no threshold: immediate re-auth prompt plus Repairs issueUpdateFailed,ApiStatus.ERROR, no reauth reasonapi.pylocationreturn {}with the status named, like the 5xx siblingcoordinator/polling.pycoordinator/locate.py_set_auth_state(failed=True): Repairs issue,EVENT_AUTH_ERROR, diagnostic sensor onnote_erroronly, auth state left alonePlus a passthrough in
location_request.pywhose WARNING and DEBUG record both called a 404 an authentication error, one line above the recordapi.pywrites about the same event.The user-visible defect: an intact login plus one device removed from the account produced a re-authentication prompt.
How
nova_request.is_credential_rejection(err)is the single home of the criterion: permanence first, then 401/403, an unreadable status keeps the conservative verdict.api._classify_nova_auth_error(from #1262) becomes its sound-path adapter instead of a second copy of the same test. Six call sites in four files read it, serving seven handlers.This is deliberately not the cleaner architecture. Giving the non-credential case its own exception class is the better design and is tracked as follow-up, not as done. Measured over the AST: ten
tryblocks catchNovaAuthError; eight carry a broadexcept Exceptionthat would swallow a new class under a name hiding the status, and the two sound-request handlers catch it in a tuple with no broad handler, where a new class would propagate uncaught. Two opposite failure modes in one change, and no per-site red test is possible because the class does not exist beforehand. Narrowing the six sites first makes that follow-up a mechanical type swap against a green guard row.Behaviour changes a reviewer should weigh
unknowninstead ofinvalid_auth, because_map_api_exc_to_error_keyclassifies on the class name andUpdateFailedcarries no "auth". Intended (the sign-in is intact), pinned by a characterisation test. A dedicated key would needstrings.jsonplus the full translation sync and is out of scope.polling.pyandlocate.pyclear the auth state and reset the transient-auth counter on their success path, before they check whether the result was empty. Sinceapi.pynow returns{}for a non-credential rejection, a deleted device takes that path. The mechanism is old (every 5xx and 429 already reaches it); what a client rejection adds is permanence: a 5xx clears up, a deleted device does not. With one deleted tracker in the list, every cycle resets the counter, so a genuinely expired sign-in on another tracker never reaches_MAX_TRANSIENT_AUTH_FAILURES. Before this PR that 404 raised the counter itself, so the escalation did happen, for the wrong reason. Fixing it means deciding what an empty result may prove about credentials, and every healthy idle BLE poll takes the same path, so the blast radius is far wider than this PR. It is documented inAGENTS.md, in both branch comments and in two test docstrings rather than left implicit.Verification
git diff -U0: no changed or added production line in the missing list.ruff check .,ruff format --check .andmypy --stricttree-wide green after every commit, not only at the end.92be8877) has not itself been reviewed, because the review is capped at two rounds by design.Not in scope
Approach B (own exception class at the source), the empty-result reset above,
location_request.py's second handler (bareraise, no output), device cleanup for a permanently missing tracker,strings.jsonand the translations.