Repository navigation
fix(logging): drop the account e-mail and device names from unattended records - #1370
Merged
Merged
Conversation
AGENTS.md forbids the account e-mail on every log level and forbids logging a cache key name, because per-account keys embed the address. Seven records in NovaApi/nova_request.py carried it: - the four AAS lifetime records of AsyncTTLPolicy (three INFO, one WARNING) passed self.username; - the DEBUG record for a failed cache read in async_on_401 passed the key (adm_token_<email>) and the exception, whose text can repeat the key; - the two DEBUG records for the namespaced mirror in _get_initial_token_async passed the key in `extra` and the exception as exc_info. The account is dropped without replacement; levels are unchanged. The cache read and mirror records now log only type(err).__name__. The new wording avoids credential words in front of a %s. tests/test_log_hygiene_nova_account.py pins this statically (no logger argument or keyword value carries the account or a key built from it; both mirror records pass no exc_info/extra) and at runtime (the AAS records and the cache read failure name no account). Against the base, 4 of its 5 tests fail; the vacuum guard passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ecord The Google Home filter on the unattended poll path logged the device name and the Google Home label at INFO when it substituted Home coordinates. AGENTS.md section 5 (b) allows a device name only at DEBUG on unattended paths. The INFO record now names the device by its id; a DEBUG companion carries the name, the id and the label. The sibling branch that preserves precise coordinates already logs at DEBUG and is unchanged. The new test in tests/test_coordinator_semantic_mappings.py fails against the base (the INFO record carries the name, not the id). The file gets its path header, and its entry leaves LEGACY_ALLOWLIST in tests/test_guard_path_header.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the push transport is down, a poll timeout raises
UpdateFailed("Location request timed out for <name>"), and Home Assistant
logs that text at ERROR through async_set_update_error. AGENTS.md section
5 (b) allows a device name only at DEBUG on unattended paths.
Both slots (last_exception and independent_error) now read
"Location request timed out for device <id>", the same wording as the INFO
record next to them. The slot logic is unchanged.
test_timeout_report_names_the_device_by_id covers both slots; against the
base both cases fail. Two docstrings that described the old text are brought
up to date; the measured statement in one of them is kept as it was.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A locate request is cancelled after every outer poll timeout, so the INFO record "Location request cancelled for <name>" recurred on an unattended path (three hits in one live log since 2026-10-15). AGENTS.md section 5 (b) allows a device name only at DEBUG there. The INFO record now names the device by its canonical id; a DEBUG companion carries the name and the id. The re-raise is unchanged. The new test in tests/test_location_request_r6_name_sweep.py injects asyncio.CancelledError into the Nova request and fails against the base. The helper _wire_nova_raises now accepts a BaseException for that purpose. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Semgrep's python-logger-credential-disclosure rule reports a format string that carries "token" in front of a %s on its first source line, whatever the argument is. Three DEBUG records matched it: - location_request.py: "No FCM token for %s; skipping this locate" and "FCM token obtained for %s (len=%d)"; - nova_request.py: "Nova API: using refreshed token for request to %s.". They now read "No push registration for %s; ...", "FCM registration ready for %s (length %d)" and "Nova API: using the refreshed authorization for the request to %s.". Levels and arguments are unchanged. Semgrep 1.180.0 reports 0 results on the three touched modules (3 before). tests/test_log_hygiene_coordinator_nova.py now also covers both Nova modules; its Semgrep mirror reports exactly these three lines against the previous commit. AGENTS.md lists the two modules for that test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up for the log hygiene changes in this branch. - tests/test_log_hygiene_nova_account.py: the static check now also looks at a format argument that is not a plain string literal (f-string, `%` expression), at per-account key properties (`self.k_*`) and at calls of `_key_variants`. A parametrised test pins that each claimed form is detected; dropping any of the three new branches fails its own case. A local alias stays out of reach and is listed in the docstring. - AGENTS.md describes the check with these forms. - polling.py: the comment now speaks about this record only; the Google Home filter module has its own records. - location_request.py and the sweep docstring: an outer poll timeout can cancel the request while it waits in this handler, not on every timeout (a timeout during the cache reads before the `try` does not reach it). - test_coordinator_status.py: "since this change" had no anchor after the merge; the docstring now says "now reads". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
Owner
Author
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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". |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch was successfully deployed
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.
Summary
Drop the account e-mail from every log record in
NovaApi/nova_request.py, and keep device names and Google Home labels out of INFO records and theUpdateFailedtext on unattended paths (they now name the device by its id; the name moves to a DEBUG companion record). Three DEBUG format strings get neutral wording so Semgrep'spython-logger-credential-disclosurerule no longer matches them.Motivation
AGENTS.mdsection 1 ("Log hygiene for scanners") and section 5 ("Never log") forbid the account e-mail on every log level and forbid logging a cache key name, because per-account keys embed the address. Section 5 (b) allows a device name above DEBUG only on paths a user triggers, not on unattended ones. Seven records innova_request.pystill carried the address (directly or as part of a key), and three records on the poll path still named the device at INFO or in theUpdateFailedtext that Home Assistant logs at ERROR.Change Log (human-readable)
Added
tests/test_log_hygiene_nova_account.py: static and runtime checks that no logger call inNovaApi/nova_request.pypasses the account name or a key built from it (arguments, keyword values such asextra, and computed format arguments), that both namespaced-mirror records in_get_initial_token_asynclog only the exception type, and that the AAS lifetime records and the cache read failure name no account at runtime.tests/test_coordinator_semantic_mappings.py,tests/test_coordinator_status.py(bothUpdateFailedslots) andtests/test_location_request_r6_name_sweep.py.Changed
NovaApi/nova_request.py: the four AAS lifetime records (three INFO, one WARNING) no longer passself.username; the cache read failure inasync_on_401and the two namespaced-mirror records logtype(err).__name__instead of the key, the exception text,extra={"cache_key": ...}andexc_info. Levels are unchanged.coordinator/polling.py: the Google Home substitution record names the device bydev_idat INFO; name, id and label go to a new DEBUG record. Both poll-timeoutUpdateFailedtexts readLocation request timed out for device <dev_id>.NovaApi/ExecuteAction/LocateTracker/location_request.py: a cancelled locate request names the device by its canonical id at INFO, the name goes to DEBUG.No push registration for %s; skipping this locate,FCM registration ready for %s (length %d),Nova API: using the refreshed authorization for the request to %s.tests/test_log_hygiene_coordinator_nova.pynow also covers both Nova modules.AGENTS.mdnames the new test and the two modules added to the existing one.tests/test_coordinator_semantic_mappings.pygets its path header, and its entry leavesLEGACY_ALLOWLISTintests/test_guard_path_header.py.Fixed
UpdateFailedon unattended paths.Removed
Performance
Security/Privacy
032a4d22: it says a cancellation follows every outer poll timeout; it can follow one, but a timeout during the cache reads before the handler'strydoes not reach this record. The code comment says so since79c4d0c1.Compatibility
manifest.json. Anyone grepping the old log texts needs the new wording.Tests (MUST)
pytest -qpasses locally: 8796 passed, 19 skipped (908 s), total coverage 82.50 % (the threshold inpyproject.tomlis 80 %).diff-coveragainst61a04838: 100 % of the executable changed lines.61a04838, 4 of the 5 tests in the new hygiene module fail (the vacuum guard passes), bothUpdateFailedslot cases fail, the Google Home and cancellation tests fail.self.usernameback, WARNING→INFO, key instead of the type name,exc_info=errback,extraback each fail at least one test; eachUpdateFailedslot mutant fails exactly its own case; each new branch of the widened account predicate fails exactly its own form case.config_flow.pynot touched by this PR. ☐ Total ≥ 95 % is not met (82.50 %), unchanged by this PR.Token Cache Safety (MUST)
Docs & i18n
UpdateFailedhas notranslation_keyanywhere in the package, not changed here).AGENTS.mdupdated as described above.Deprecation Check (concise)
UpdateFailedand the coordinator's error logging)UpdateFailed.Risk & Rollback
61a04838..79c4d0c1); no data, entity IDs or cache keys change.Local Verification (run before pushing)
ruff format --check): rc 0; 178 guard tests, Semgrep mirror and alias mutants green.ruff format --check .andruff check .: clean.mypy --strictovercustom_components/googlefindmyandtests: 557 files, no issues.python-logger-credential-disclosure0 results (3 before).pre-commit run --all-files: the same hooks fail on61a04838and on this branch, with the same bandit (3) and codespell (582) findings; the failing hooks are environment issues here (hook environments withoutpytest/mypy, legacy ruff alias). Nothing new from this diff.script/local_verify.py --all --skip-pytest: no stage failed.hassfestwas not run locally (no Docker); CI runs it.Reviewer Notes (optional)
_get_initial_token_asyncsit in# pragma: no coverbranches; they are pinned statically, not at runtime.google_home_filter.pystill logs the Google Home label at INFO and WARNING; eight older poll-path records above DEBUG still carry the device name (documented exception inAGENTS.mdsection 5 (b)).🤖 Generated with Claude Code