From a9bcc72e6168ca7c13d45243eb2319e7e06db50c Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Sun, 23 Aug 2026 20:33:50 +0200 Subject: [PATCH] Count the records that never decrypted, instead of losing them between two lines Pin moves 4f940158 -> ddc7f234, which carries three fixes from jamorenom's review of #144 plus the merge of ubrt's `current_keys()` work. **`fetch` built its "not exportable" list by walking what came back.** Anything `decrypt_records` dropped was therefore absent from that list *and* from the candidates at once, and the user got a shorter table with nothing to explain it. The comment two lines below has been saying why that is bad the whole time: Named rather than dropped quietly: "fewer tags than expected" and "some of those were never tags" look identical from outside. Which is exactly right, and the layer above it was dropping quietly. This is the residue of #89's fix - that bug was one unreadable item ending the whole export, fixed by skipping - and it was reported by the same person, who had to count raw records by hand to notice. FindMy.py had always computed the tally and logged it; it now returns it, so `Fetched` carries `undecryptable` and `first_miss`. **Counted separately from `skipped`, and not phrased as a failure.** Those are different things: `skipped` is per accessory and names one, because something was read and set aside for a stated reason. These were never read - there is no beacon id to name, the id being inside what would not open - so folding them in would invent rows. And a zone legitimately holds records belonging to other parties, so a non-zero count is usually nothing. Both surfaces say so, and say the one thing that would make it worth a second look: it being close to the number of tags somebody expected. `first_miss` rides along because the count cannot tell "somebody else's records" from "we are comparing keys in the wrong encoding", and those lead in opposite directions. Five tests, verified by breaking three things: dropping the count, counting only the first reason (the old undercount), and folding them into the not-exportable rows - three, one and one red. Driven with `asyncio.run` like the rest of the suite; there is no pytest-asyncio here. Also in the pin, unused by the exporter but now available: a 401 on a session with no password raises `UnauthorizedError` rather than a bare `ValueError` three frames down, and a secondary-key match no longer ratchets alignment forward past the accessory. The app does not reach that second one - it drives `_fetch_key_reports` with its own `index_by_key`, built from `keys_between`, which dedupes ascending and so already holds the lowest index. 560 exporter tests and 227 bridge tests pass. All four pins moved together, per rule 14. Co-Authored-By: Claude Opus 5 --- app/build.gradle.kts | 2 +- app/src/test/python/requirements.txt | 2 +- python/exporter/cli.py | 15 +++ python/exporter/icloud.py | 44 ++++++++- python/exporter/wizard.py | 12 +++ python/pyproject.toml | 2 +- python/test/test_undecryptable_count.py | 118 ++++++++++++++++++++++++ python/uv.lock | 4 +- 8 files changed, 193 insertions(+), 6 deletions(-) create mode 100644 python/test/test_undecryptable_count.py diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 0b5520f4..d9b28a11 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -404,7 +404,7 @@ chaquopy { // wheel for desktop platforms and a pure-Python `py3-none-any` one as well. // There is no Android wheel, so pip falls back to the pure-Python build - which // is correct but markedly slower. The messages here are small enough not to care. - install("git+https://github.com/parawanderer/FindMy.py@4f940158c437b17ae61a8e90af084d658750629c") + install("git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d") install("NSKeyedUnArchiver==1.5") diff --git a/app/src/test/python/requirements.txt b/app/src/test/python/requirements.txt index b96abbc8..a365cb9e 100644 --- a/app/src/test/python/requirements.txt +++ b/app/src/test/python/requirements.txt @@ -9,7 +9,7 @@ # `FindMy==0.9.8` here for as long as the app built the fork, so every bridge test # ran against a library the app does not ship - which is not a small difference: # the fork's Anisette providers take `serial=` and PyPI's do not. -git+https://github.com/parawanderer/FindMy.py@4f940158c437b17ae61a8e90af084d658750629c +git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d NSKeyedUnArchiver==1.5 PyYAML==6.0.2 diff --git a/python/exporter/cli.py b/python/exporter/cli.py index 6ec77114..6cb21399 100644 --- a/python/exporter/cli.py +++ b/python/exporter/cli.py @@ -831,6 +831,21 @@ async def run(arguments: argparse.Namespace) -> int: for skipped in fetched.skipped: print(f"Not exportable: {skipped.beacon_id} {skipped.reason}", file=sys.stderr) + if fetched.undecryptable: + # **Not phrased as a failure**, because it usually is not one: a zone holds records + # belonging to other parties too. It is said at all because the alternative is a + # shorter list with nothing to explain it, and "fewer tags than expected" reads + # exactly like "some of those were never tags". + print( + f"\n{fetched.undecryptable} record(s) in the account could not be read, and are" + " not in the list above.", + file=sys.stderr, + ) + print("Records belonging to somebody else look like this too, so this is often" + " nothing. It is", file=sys.stderr) + print("worth a second look only if it is close to the number of tags you expected" + " to see.", file=sys.stderr) + if fetched.candidates: picked = ( _take_all(fetched.candidates, include_my_devices=arguments.include_my_devices) diff --git a/python/exporter/icloud.py b/python/exporter/icloud.py index a1c15fe1..fe9a804e 100644 --- a/python/exporter/icloud.py +++ b/python/exporter/icloud.py @@ -132,6 +132,34 @@ class Fetched: candidates: list[Candidate] skipped: list[Skipped] + undecryptable: int = 0 + """ + How many records in the zone did not decrypt at all. + + **Counted separately from `skipped`, and not as a failure.** `skipped` is per accessory and + names one: something was read, understood, and set aside for a stated reason. These were never + read - they have no beacon id to name, because the id is inside what would not open. + + **A non-zero count is not necessarily wrong.** A zone legitimately holds records belonging to + other parties, which this account has no key for and never should. So it is reported as a + number and a caveat rather than as an error, and the only thing that would make it alarming is + it being close to the number of tags somebody expected. + + It exists because it was invisible: FindMy.py counted these, logged the tally and dropped it, + and `fetch` builds its `skipped` list by walking what came back - so anything lost here was + absent from both lists at once. "Fewer tags than expected" and "some of those were never tags" + look identical from outside, which is the same sentence the per-accessory list exists for. + """ + + first_miss: str | None = None + """ + The first "no key held", in full, when there was one. + + The count alone cannot tell the two causes apart: records that genuinely belong to somebody + else, and this client comparing keys in the wrong encoding. Those lead in opposite directions, + and only the message says which. + """ + @dataclass(frozen=True) class ClientIdentity: @@ -550,7 +578,21 @@ async def fetch(client: AsyncFindMyClient) -> Fetched: candidates.append(_candidate(group.beacon, group.naming, group.alignment)) - return Fetched(candidates=candidates, skipped=skipped) + if decrypted.skipped_total: + # Reported, not raised. See `Fetched.undecryptable` for why a count here is not + # automatically wrong - and why it still must not be silent. + logger.info( + "%d record(s) in the zone did not decrypt: %s", + decrypted.skipped_total, + ", ".join(f"{reason} x{count}" for reason, count in decrypted.skipped.items()), + ) + + return Fetched( + candidates=candidates, + skipped=skipped, + undecryptable=decrypted.skipped_total, + first_miss=decrypted.first_miss, + ) def _why_not_locatable(beacon: DecryptedRecord) -> str: diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index 5ee251b6..feaddea8 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -147,6 +147,7 @@ def __init__(self, *args, **kwargs) -> None: # What the source could not export, kept rather than passed around: the list is redrawn # whenever a key file is added, and that redraw knows nothing about the original read. self.skipped: list = [] + self.undecryptable = 0 self.route = source.detect() self._build() @@ -396,6 +397,7 @@ def _load(self) -> None: return self.candidates = fetched.candidates + self.undecryptable = fetched.undecryptable # Read once. A second read would sign in again, register nothing new and rebuild the list # under the ticks somebody has already made. self.read_button.configure(state="disabled") @@ -583,6 +585,16 @@ def _say_where_things_stand(self) -> None: f"{len(self.skipped)} record(s) could not be exported: they carry no key material.", ) + if self.undecryptable: + # Deliberately not "could not be exported", which is what the line above means and is + # a different thing: those were read and set aside, these were never read at all. + # Records belonging to other people look like this too, so it says so rather than + # implying something went wrong. + notes.append( + f"{self.undecryptable} record(s) could not be read, and are not listed - often" + " somebody else's, so this is usually nothing.", + ) + if not self.choices.get_children(): # The empty list is the first thing anybody sees now, so it has to say what the two # buttons are for - including that the second one needs no Apple account, which is the diff --git a/python/pyproject.toml b/python/pyproject.toml index fbc09a45..f01ddc0f 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -69,7 +69,7 @@ constraint-dependencies = [ ] [tool.uv.sources] -FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "4f940158c437b17ae61a8e90af084d658750629c" } +FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" } [dependency-groups] # Only the release build installs this, with `uv sync --no-default-groups --group build`. It is diff --git a/python/test/test_undecryptable_count.py b/python/test/test_undecryptable_count.py new file mode 100644 index 00000000..89ad0255 --- /dev/null +++ b/python/test/test_undecryptable_count.py @@ -0,0 +1,118 @@ +""" +Records that never decrypted, and why the exporter has to mention them. + +**`fetch` builds its "not exportable" list by walking what came back**, so anything +`decrypt_records` dropped was absent from that list and from the candidates at once - it +disappeared between two lines and the user saw a shorter table with nothing to explain it. + +This is the residue of issue #89's fix. That bug was *one unreadable item ends the whole export*, +fixed by skipping. Skipping silently is the other half of the same mistake, and it was reported by +the same person who found #89's cause - who had to count raw records by hand to notice. + +**A count here is not automatically wrong**, which is the reason it is reported as a number and a +caveat rather than raised: a zone legitimately holds records belonging to other parties. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +from exporter import icloud + + +class FakeDecrypted(list): + """Stands in for FindMy.py's `DecryptedRecords`, which is a list carrying a tally.""" + + def __init__(self, records=(), *, skipped=None, first_miss=None): + super().__init__(records) + self.skipped = dict(skipped or {}) + self.first_miss = first_miss + + @property + def skipped_total(self) -> int: + return sum(self.skipped.values()) + + +@pytest.fixture +def account(monkeypatch): + """`fetch` with its Apple half replaced, returning whatever tally a test wants.""" + def _fetch(*, skipped=None, first_miss=None): + decrypted = FakeDecrypted(skipped=skipped, first_miss=first_miss) + + monkeypatch.setattr(icloud, "decrypt_records", lambda *_a, **_k: decrypted) + monkeypatch.setattr(icloud, "group_records", lambda _d: []) + + return decrypted + + return _fetch + + +def run_fetch(client): + """Driven with `asyncio.run`, as the rest of this suite does - there is no pytest-asyncio.""" + return asyncio.run(icloud.fetch(client)) + + +class FakeClient: + """The three calls `fetch` makes before it starts counting.""" + + class _Store: + async def zone_record_defaults(self, _keys): + return {} + + def __init__(self): + self.store = self._Store() + + async def zone_keys(self): + return {} + + async def records(self): + return [] + + +class TestTheCountReachesTheCaller: + def test_a_clean_read_reports_nothing(self, account): + account() + + fetched = run_fetch(FakeClient()) + + assert fetched.undecryptable == 0 + assert fetched.first_miss is None + + def test_every_reason_is_counted_not_just_the_first(self, account): + # Two different failure kinds. The tally used to be logged as a whole and dropped, so a + # caller that guessed from one of them would undercount the other. + account(skipped={"MissingKeyError": 4, "PCSError": 2}) + + fetched = run_fetch(FakeClient()) + + assert fetched.undecryptable == 6 + + def test_the_first_miss_is_carried(self, account): + # The count alone cannot tell "somebody else's records" from "we are comparing keys in + # the wrong encoding", and those lead in opposite directions. + account(skipped={"MissingKeyError": 1}, first_miss="no key held for ABC") + + fetched = run_fetch(FakeClient()) + + assert fetched.first_miss == "no key held for ABC" + + def test_it_does_not_become_a_not_exportable_row(self, account): + # `skipped` is per accessory and names one. These have no beacon id to name - the id is + # inside the thing that would not open - so folding them in would invent rows. + account(skipped={"MissingKeyError": 3}) + + fetched = run_fetch(FakeClient()) + + assert fetched.skipped == [] + assert fetched.undecryptable == 3 + + def test_it_is_not_raised(self, account): + # A zone holds other parties' records legitimately, so this must not fail an export that + # otherwise worked. + account(skipped={"MissingKeyError": 99}) + + fetched = run_fetch(FakeClient()) + + assert fetched.undecryptable == 99 diff --git a/python/uv.lock b/python/uv.lock index 8cde8c92..241917a1 100644 --- a/python/uv.lock +++ b/python/uv.lock @@ -659,7 +659,7 @@ wheels = [ [[package]] name = "findmy" version = "0.10.1" -source = { git = "https://github.com/parawanderer/FindMy.py?rev=4f940158c437b17ae61a8e90af084d658750629c#4f940158c437b17ae61a8e90af084d658750629c" } +source = { git = "https://github.com/parawanderer/FindMy.py?rev=ddc7f2342fc9f32ebe315b85c22a4554ce419f6d#ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" } dependencies = [ { name = "aiohttp" }, { name = "anisette" }, @@ -1032,7 +1032,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=4f940158c437b17ae61a8e90af084d658750629c" }, + { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=ddc7f2342fc9f32ebe315b85c22a4554ce419f6d" }, { name = "pycryptodome", specifier = "==3.22.0" }, { name = "pyyaml", specifier = "==6.0.2" }, { name = "pyzipper", specifier = "==0.4.0" },