From 8f9ad9cb358e87d222f0636de55eb5fbf39f5a5d Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:12:51 +0200 Subject: [PATCH 01/11] Remove the wizard's "Lock with a code" checkbox A bundle holds key material that cannot be revoked - the only way to withdraw an exported accessory is to unpair it - and it travels through a mail account or a chat app and outlives the conversation by years. It was guarded by a ticked checkbox, which is one idle click from an unlocked zip. That click gets made: a user sent @parawanderer their tags in an unlocked bundle. Not an attack and not ignorance of the stakes - it is what an unticked box in the corner of a window produces, eventually, from somebody hurrying. The person the lock protects is exactly the person who would untick it to make a message go away. So the control is gone rather than defaulted on. _write_it calls generate_passcode() unconditionally, and the "this bundle is not locked" branch goes with it, because nothing can reach it. The escape hatch stays on the CLI as --no-password, and its being CLI-only is the point rather than an oversight. Somebody who found a flag, read what it does and typed it has chosen an unlocked bundle. Somebody clicking through a window has not, and giving both the same affordance treats those as the same decision. The release-ordering argument that once justified an opt-out is spent: it protected a recipient on an app too old to open a locked bundle, and every release up to 1.0.5 is refused at sign-in by Apple's edge, so no such recipient exists. The tests assert the control's absence rather than its default - by attribute, and by walking the widgets, so renaming the field and keeping the checkbox fails too. Rule 9 updated; it described the old default. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 16 ++++- python/exporter/wizard.py | 72 +++++++++++--------- python/test/test_wizard_bundle_locking.py | 82 ++++++++++++++--------- 3 files changed, 103 insertions(+), 67 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5ea3843e..240cb664 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -163,8 +163,20 @@ message about the zip rather than about a code. Publish an exporter that locks b that app is out, and every bundle written that day is unopenable by whoever receives it, and the recipient is the one person in that transaction who chose none of it and can fix none of it. -**The wizard's lock is defaulted on**, in `wizard.py`'s `lock_bundle`, and -`test_wizard_bundle_locking.py` asserts that. +**The wizard has no lock switch at all**, and that is stronger than a default. `_write_it` calls +`generate_passcode()` unconditionally, so no path from that window produces an unlocked bundle; +`test_wizard_bundle_locking.py` asserts the control's *absence*, by attribute and by walking the +widgets, rather than asserting a value somebody can flip back. + +It was a ticked checkbox, and a ticked checkbox is one idle click from an unlocked zip holding +key material that cannot be revoked. That click gets made: a user sent @parawanderer their tags +in an unlocked bundle. **Do not restore it** — the person the lock protects is precisely the +person who would untick it to make a message go away. + +The escape hatch is `exporter.cli`'s `--no-password`, and its being CLI-only is the design rather +than an omission. Somebody who found a flag and typed it has chosen an unlocked bundle; somebody +clicking through a window has not, and offering both the same control treats those as one +decision. **It was flipped on before app 1.1.0 was published, which is not what the paragraph above describes, and the exception is worth understanding rather than copying.** The ordering exists to diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index 5cc0ca68..b7512f07 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -248,24 +248,29 @@ def _build(self) -> None: self.confirm_button = ttk.Button(buttons, text="Export…", command=self._export, state="disabled") self.confirm_button.grid(row=0, column=4) - # **On by default, and the default is the whole point.** A bundle holds key material that - # cannot be revoked - the only way to withdraw an exported accessory is to unpair it - and - # it then travels through a mail account or a chat app and outlives the conversation by - # years, sitting in a backup long after everyone has forgotten it is there. Whoever most - # needs the lock is whoever would never go looking for a checkbox to turn it on. + # **There is no "Lock with a code" checkbox here, and removing it was the point.** # - # Off until app 1.1.0 was released, because nothing older can decrypt a locked bundle at - # all and the person who met that failure was the recipient. That app is out, so this is - # back on - see AGENTS.md rule 9 for why the two releases are ordered. + # A bundle holds key material that cannot be revoked - the only way to withdraw an + # exported accessory is to unpair it - and it then travels through a mail account or a + # chat app and outlives the conversation by years, sitting in a backup long after + # everyone has forgotten it is there. # - # The opt-out stays for the recipient still running something older, which is most of them - # on any given day after a release. - self.lock_bundle = tk.BooleanVar(value=True) - ttk.Checkbutton( - buttons, - text="Lock with a code", - variable=self.lock_bundle, - ).grid(row=1, column=4, sticky="e", pady=(6, 0)) + # It was a ticked checkbox, which is one idle click away from an unlocked bundle, and + # that click has been made: a user sent @parawanderer their tags in an unlocked zip. + # Not as an attack and not through ignorance of the consequences - it is simply what an + # unticked box in a corner produces, eventually, from somebody hurrying. The person best + # served by the lock is exactly the person who would untick it to make an error message + # go away. + # + # The escape hatch lives on the CLI, as `--no-password`, and that is deliberate rather + # than an oversight here. Somebody who found a flag, read what it does and typed it has + # demonstrably chosen an unlocked bundle. Somebody clicking through a window has not, and + # giving both the same affordance treats those as the same decision. + # + # The ordering argument that once justified an opt-out is spent: see AGENTS.md rule 9. + # It protected a recipient running an app too old to open a locked bundle, and no such + # recipient exists - every release up to 1.0.5 is refused at sign-in by Apple's edge, so + # an unlocked bundle buys its owner nothing. def _save_logs(self) -> None: """ @@ -783,20 +788,26 @@ def _export(self) -> None: def _write_it(self, bundle: ExportBundle, path: str, count: int) -> bool: """ - Write the zip, lock it unless told otherwise, and say what happened. + Write the zip, always locked, and say what happened. + + **Always, with no way to ask otherwise from this window.** It was a ticked checkbox until + app 1.1.0 shipped and then briefly afterwards, and what the checkbox actually produced + was unlocked bundles: one idle click, on the control that decides whether irrevocable key + material travels in the clear. Somebody has already sent their tags to a stranger that + way. See the comment where the checkbox used to be built. - **Locked by default, which it was not until app 1.1.0 existed.** What blocked it was - release ordering rather than a missing feature: before zip4j the app could not decrypt - anything at all, so a locked bundle was a file nobody\'s installed app could open, and the - people worst affected were recipients, who did not choose the exporter\'s version and - could not fix it from their side. + Release ordering was what once justified an opt-out - before zip4j the app could not + decrypt anything, so a locked bundle was a file no recipient could open. That is spent: + 1.1.0 reads them, and every release before it is refused at sign-in by Apple regardless. - 1.1.0 reads them, so the default flips. The checkbox stays for the versions before it. + `exporter.cli` keeps `--no-password` for the case that genuinely needs it. The difference + is not the capability but who is asking: a flag somebody looked up is a decision, a box + in the corner of a window is not. :returns: whether the window should close. False leaves it open on a failure, so the export can be retried without starting over. """ - passcode = generate_passcode() if self.lock_bundle.get() else None + passcode = generate_passcode() try: write_zip(bundle, path, password=passcode) @@ -812,15 +823,10 @@ def _write_it(self, bundle: ExportBundle, path: str, count: int) -> bool: messagebox.showerror("That bundle could not be written", str(e)) return False - if passcode is None: - messagebox.showinfo( - "Exported", - f"{count} accessory(s) written to:\n{path}\n\n" - "This bundle is not locked. Anyone who has the file can locate these tags, and" - " that cannot be undone.", - ) - else: - _show_the_code(self, path, count, passcode) + # No unlocked branch: `generate_passcode` always returns one, so there is no path from + # this window to a bundle without a code, and nothing here has to explain what an + # unlocked bundle means. The CLI's `--no-password` still has that explanation. + _show_the_code(self, path, count, passcode) return True diff --git a/python/test/test_wizard_bundle_locking.py b/python/test/test_wizard_bundle_locking.py index eb444f6c..c5abee40 100644 --- a/python/test/test_wizard_bundle_locking.py +++ b/python/test/test_wizard_bundle_locking.py @@ -1,13 +1,15 @@ """ -The wizard can lock the bundles it writes, and shows the code once. +The wizard locks every bundle it writes, and shows the code once. -**The default is on, now that app 1.1.0 is released.** A locked bundle can only be opened by that -version or newer; anything older fails with a message about the zip rather than about a code, and -the person who meets that failure is the recipient - who chose neither the exporter nor its -version. That is why this waited for the app rather than shipping alongside it. +**There is no way to ask it not to, and that is the behaviour under test.** It was a ticked +checkbox, which is one idle click from an unlocked bundle holding key material that cannot be +revoked - and that click has been made in the field, by somebody who sent their tags to a +stranger in an unlocked zip. The escape hatch lives on the CLI's `--no-password`, where finding +a flag and typing it is evidence of a decision. -**These tests assert the default in both directions on purpose.** The value moved twice with no -test noticing either time, which is how it came to be wrong in the first place. +**Asserted as the absence of a control, not only as a default.** A default is a value somebody +can flip back; this suite fails if the window grows a way to turn locking off at all. The value +moved twice before without a test noticing either time. The code is the part with a permanent cost. It is not stored anywhere and cannot be recovered, so a bundle written without the user being shown its code is a bundle nobody can ever open. @@ -47,10 +49,8 @@ def bundle(): return ExportBundle(entries={"OPENTAGVIEWER.yml": b"version: 0.0.2\n"}, exported_at_ms=0) -def write(window, bundle, path, *, locked: bool): - """Run the write step with the checkbox in a known state, and report what happened.""" - window.lock_bundle.set(locked) - +def write(window, bundle, path): + """Run the write step and report what happened. There is no state to set: it always locks.""" with mock.patch.object(wizard, "write_zip") as write_zip, \ mock.patch.object(wizard, "_show_the_code") as shown, \ mock.patch.object(wizard.messagebox, "showinfo") as info, \ @@ -60,32 +60,49 @@ def write(window, bundle, path, *, locked: bool): return write_zip, shown, info, error, closed -class TestTheDefault: +class TestThereIsNoWayToTurnItOff: """ - Off until an app that can open one is released - see the module docstring. - - A bundle holds key material that cannot be revoked and travels through other people's - infrastructure, so on is where this belongs eventually. It is not there yet. + The control is gone, not merely defaulted on - see the module docstring for what that cost. """ - def test_the_checkbox_starts_ticked(self, window): - assert window.lock_bundle.get() is True, ( - "the default is on now that app 1.1.0 is released and can open a locked bundle" + def test_the_window_has_no_locking_switch(self, window): + assert not hasattr(window, "lock_bundle"), ( + "the window grew a way to turn locking off again; the CLI's --no-password is where" + " that belongs, because typing a flag is a decision and clicking a box is not" + ) + + def test_no_checkbox_offers_it_either(self, window): + # The attribute could be renamed and the checkbox kept, which would pass the test above + # while putting the click back on screen. So the widgets are searched as well. + labels = [] + + def walk(widget): + for child in widget.winfo_children(): + try: + labels.append(str(child.cget("text")).lower()) + except tk.TclError: + pass + walk(child) + + walk(window) + + assert not any("lock" in label for label in labels), ( + f"something on the window still offers locking as a choice: {labels}" ) def test_a_bundle_is_written_with_a_code(self, window, bundle, tmp_path): write_zip, _shown, _info, _error, _closed = write( - window, bundle, tmp_path / "x.zip", locked=True) + window, bundle, tmp_path / "x.zip") passcode = write_zip.call_args.kwargs["password"] - assert passcode, "the bundle was written unlocked while the box was ticked" + assert passcode, "the bundle was written unlocked" assert len(passcode) == 12 def test_the_code_uses_the_alphabet_the_importer_expects(self, window, bundle, tmp_path): # Crockford's base32, minus I, L, O and U. The app folds the confusable letters back on # input; a code containing one would still work, but it would defeat the point of the # alphabet - which is that this gets read off a screen and typed somewhere else. - write_zip, *_ = write(window, bundle, tmp_path / "x.zip", locked=True) + write_zip, *_ = write(window, bundle, tmp_path / "x.zip") assert set(write_zip.call_args.kwargs["password"]) <= set( "0123456789ABCDEFGHJKMNPQRSTVWXYZ") @@ -99,18 +116,21 @@ class TestShowingTheCode: def test_the_code_is_shown_and_it_is_the_one_that_was_used(self, window, bundle, tmp_path): write_zip, shown, _info, _error, _closed = write( - window, bundle, tmp_path / "x.zip", locked=True) + window, bundle, tmp_path / "x.zip") shown.assert_called_once() assert shown.call_args.args[3] == write_zip.call_args.kwargs["password"] - def test_an_unlocked_bundle_says_so_instead(self, window, bundle, tmp_path): - write_zip, shown, info, _error, _closed = write( - window, bundle, tmp_path / "x.zip", locked=False) + def test_the_code_is_always_shown_because_there_is_always_one(self, window, bundle, tmp_path): + # There used to be an "Exported, and this bundle is not locked" path here. It is gone + # with the checkbox: every write from this window has a code, so every write shows one. + # If a no-code path ever comes back, this fails rather than silently writing a bundle + # whose only warning nobody wrote. + write_zip, shown, info, _error, _closed = write(window, bundle, tmp_path / "x.zip") - assert write_zip.call_args.kwargs["password"] is None - shown.assert_not_called() - assert "not locked" in info.call_args.args[1] + assert write_zip.call_args.kwargs["password"] is not None + shown.assert_called_once() + info.assert_not_called() class TestWhenItCannotBeWritten: @@ -119,7 +139,6 @@ class TestWhenItCannotBeWritten: """ def test_a_missing_pyzipper_is_said_plainly(self, window, bundle, tmp_path): - window.lock_bundle.set(True) with mock.patch.object(wizard, "write_zip", side_effect=RuntimeError("pyzipper is not installed")), \ @@ -130,7 +149,6 @@ def test_a_missing_pyzipper_is_said_plainly(self, window, bundle, tmp_path): assert "pyzipper" in error.call_args.args[1] def test_a_disk_that_will_not_take_it_keeps_the_window(self, window, bundle, tmp_path): - window.lock_bundle.set(True) with mock.patch.object(wizard, "write_zip", side_effect=OSError("No space left")), \ mock.patch.object(wizard.messagebox, "showerror") as error: @@ -140,7 +158,7 @@ def test_a_disk_that_will_not_take_it_keeps_the_window(self, window, bundle, tmp assert "No space left" in error.call_args.args[1] def test_a_successful_write_does_close_it(self, window, bundle, tmp_path): - *_rest, closed = write(window, bundle, tmp_path / "x.zip", locked=True) + *_rest, closed = write(window, bundle, tmp_path / "x.zip") assert closed is True From bf5b817c1d48af8ede76139b7f4d2193f90da67e Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:14:33 +0200 Subject: [PATCH 02/11] Let a dead CloudKit token reach the sign-in-again screen Issue #225, and it is a dead end rather than a wrong sentence. A CloudKit 401 arrived as UNKNOWN, which is the retry screen, and retrying re-runs the identical call. So after a failed sign-in, the Settings button that reconnects the account met the same "could not check your account just now" every time, with nothing on screen offering a way back - in the one flow whose entire job is recovering from this. The only escape was unlinking the account, which nothing says. CREDENTIALS_REJECTED already does the right thing at the other end: it signs out and drops the stored blob so the next screen does not meet the same wall. The failure simply never reached it. UnauthorizedError went unclassified because the type means two opposite things: request_pet raises it when a second factor is being demanded, which is answered with a code and not a sign-out. That is still true and still matters, so this matches the wording rather than the type. Both CloudKit 401 sites in findmy/cloudkit/client.py open with "CloudKit rejected the" and both go on to say to log in again; the request_pet ones are about re-authentication ending in the wrong state and share none of it. Matching a message is unpleasant for the reasons the ValueError above it already documents, and is done for the same reason: the alternative is a screen nobody can leave. A distinct exception type in the fork would be better and is worth doing when the pin next moves. Three tests: both CloudKit wordings sign out, and a 2FA demand emphatically does not. The first two fail without the change. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/main/python/icloud_bridge.py | 35 +++++++++++++++--- app/src/test/python/test_icloud_bridge.py | 45 +++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/app/src/main/python/icloud_bridge.py b/app/src/main/python/icloud_bridge.py index 371fe07b..c2085031 100644 --- a/app/src/main/python/icloud_bridge.py +++ b/app/src/main/python/icloud_bridge.py @@ -44,7 +44,11 @@ import identity as app_identity from exporter import icloud from exporter.identity import written_by_opentagviewer -from findmy.errors import AppleServiceUnavailableError, InvalidCredentialsError +from findmy.errors import ( + AppleServiceUnavailableError, + InvalidCredentialsError, + UnauthorizedError, +) from findmy.keychain.enrolment import DeviceDescription from findmy.keychain.join import JoinedPeer from findmy.keychain.recovery import RecoveryError @@ -228,15 +232,34 @@ def _needsAFreshSignIn(error: BaseException | None) -> bool: the app's most ordinary auth failure arriving as `UNKNOWN` and being offered a retry that cannot work. The string is checked narrowly, and `account.py` raises it in exactly one place. - **`UnauthorizedError` is deliberately not here.** It means two different things depending on - where it came from - `request_pet` raises it when a second factor is being demanded, which the - app answers by asking for a code rather than signing out, while CloudKit raises the same type - for a genuine 401. Treating a 2FA prompt as a dead session would cost somebody a sign-in they - did not need, so it stays unclassified until the two can be told apart. + **`UnauthorizedError` counts, but only the CloudKit half of it.** The type means two opposite + things depending on where it came from: `request_pet` raises it when a second factor is being + demanded, which the app answers with a code rather than a sign-out, while CloudKit raises the + same type for a genuine 401 on a token that has expired. Treating a 2FA prompt as a dead + session would cost somebody a sign-in they did not need, which is why this was left + unclassified for a long time. + + **Leaving it unclassified turned out to cost more.** A dead CloudKit token reached the screen + as `UNKNOWN`, which is the retry screen - and retrying re-runs the identical call, so the + Settings button that reconnects the account led to the same failure every time with no way + back. Not a misleading message: a dead end, in the one flow whose whole job is recovering + from this. Reported in issue #225. + + **The two are distinguishable, and narrowly.** Both CloudKit 401 sites in + `findmy/cloudkit/client.py` open with "CloudKit rejected the" and both go on to say to log in + again; the `request_pet` ones are about re-authentication ending in the wrong state and share + no wording with them. Matching a message is unpleasant for the same reasons the `ValueError` + above is, and is done for the same reason - the alternative is a screen nobody can leave. """ if _isCausedBy(error, InvalidCredentialsError): return True + # Narrow on purpose: the prefix both CloudKit sites share, and nothing broader. A bare + # `UnauthorizedError` check here would swallow the 2FA demand as well and sign people out + # mid-flow. + if _isCausedBy(error, UnauthorizedError) and _saysAnyOf(error, ("CloudKit rejected the",)): + return True + # `not self._username or not self._password` in `_gsa_authenticate`, which is reached by # anything that tries to re-authenticate a restored session. return _isCausedBy(error, ValueError) and _saysAnyOf( diff --git a/app/src/test/python/test_icloud_bridge.py b/app/src/test/python/test_icloud_bridge.py index 4819ace9..97dd87d6 100644 --- a/app/src/test/python/test_icloud_bridge.py +++ b/app/src/test/python/test_icloud_bridge.py @@ -1063,6 +1063,51 @@ def test_a_session_restored_without_a_password_is_the_same_situation(self): assert answer["reason"] == icloud_bridge.REASON_CREDENTIALS_REJECTED + def test_a_dead_cloudkit_token_is_the_same_situation(self): + """ + **Issue #225, and it was a dead end rather than a wrong sentence.** + + A CloudKit 401 arrived as UNKNOWN, which is the retry screen, and retrying re-runs the + identical call. The Settings button that reconnects the account therefore led to the + same failure every time, with nothing on screen offering a way back - in the one flow + whose entire job is recovering from this. + """ + try: + raise UnauthorizedError( + "CloudKit rejected the iCloud token. It has most likely expired;" + " logging in again will obtain a fresh one.") + except UnauthorizedError: + answer = json.loads(icloud_bridge._unexpected("checking the account")) + + assert answer["reason"] == icloud_bridge.REASON_CREDENTIALS_REJECTED + + def test_the_other_cloudkit_refusal_is_too(self): + # The second of the two 401 sites, worded differently and meaning the same thing. The + # match is on the prefix they share rather than on either sentence. + try: + raise UnauthorizedError("CloudKit rejected the token for this operation; log in again") + except UnauthorizedError: + answer = json.loads(icloud_bridge._unexpected("fetching the beacons")) + + assert answer["reason"] == icloud_bridge.REASON_CREDENTIALS_REJECTED + + def test_a_second_factor_being_demanded_is_emphatically_not(self): + """ + **The reason this type went unclassified for so long, and the thing not to break.** + + `request_pet` raises the same class when Apple wants a code. Answering that with a + forced sign-out costs somebody a working session and a re-login they never needed - so + the match is the CloudKit wording, not the type. + """ + try: + raise UnauthorizedError( + "Re-authentication ended in state LoginState.REQUIRE_2FA rather than" + " AUTHENTICATED, so no PET was issued.") + except UnauthorizedError: + answer = json.loads(icloud_bridge._unexpected("opening a keychain session")) + + assert answer["reason"] != icloud_bridge.REASON_CREDENTIALS_REJECTED + def test_it_is_found_underneath_a_wrapper(self): # These come back through run_until_complete and FindMy.py's own layers, so the # interesting error is rarely the outermost one. From 2968df55d59379a43399a3e798916c9598ec90e7 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:19:30 +0200 Subject: [PATCH 03/11] Sign every CI debug build with one key A runner has no ~/.android/debug.keystore, so AGP generates one per run and every debug APK this repository publishes is signed by a different key. Two things follow. A debug APK cannot be installed over one from another run - same applicationId, different key, INSTALL_FAILED_UPDATE_INCOMPATIBLE - and the only way forward is an uninstall, which with allowBackup false destroys that device's imported beacons and location history. Testing successive builds has meant wiping the app every time. And Google Maps renders blank, because a Maps key is restricted by package name and signing SHA-1 together, and a SHA-1 that changes every build cannot be whitelisted. That reads as an API key missing from the build, which is how it was reported. CI now writes one keystore from DEBUG_KEYSTORE_BASE64 and the debug signing config uses app/debug-keystore.jks when it is there. Absent, Gradle logs a line and falls back to the generated key, so a fork still builds - and so a missing keystore does not present as an error when maps later come up blank. The secret goes through `env` rather than into the run block: the secrets context is not available in a step-level `if` at all, and a ${{ }} inside a script puts the value on the command line. The passwords are Android's well-known debug constants on purpose. The key proves nothing and guards nothing; giving it real secrets would only add another thing to supply before the project compiles. *.jks was already ignored. Whoever has a debug build installed needs one more uninstall, once: it was signed with a per-run key that no longer exists. SHA-1 6F:F6:8F:EB:74:AA:3E:1D:DB:8A:C9:49:10:0C:2F:FF:4A:64:CE:33, to be whitelisted against the Maps key. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build-debug.yml | 52 +++++++++++++++++++++++++++++++ CONTRIBUTING.md | 40 ++++++++++++++++++++++++ app/build.gradle.kts | 40 ++++++++++++++++++++++++ 3 files changed, 132 insertions(+) diff --git a/.github/workflows/build-debug.yml b/.github/workflows/build-debug.yml index f4858727..c5e4595a 100644 --- a/.github/workflows/build-debug.yml +++ b/.github/workflows/build-debug.yml @@ -78,6 +78,32 @@ jobs: run: | echo "MAPS_API_KEY=${{ secrets.MAPS_API_KEY || 'maps_key_default_value' }}" > secrets.properties + # **So every debug APK this repository publishes is signed by the same key.** + # + # A runner has no ~/.android/debug.keystore, so AGP generates one per run. That makes each + # debug APK unupgradeable over the last (INSTALL_FAILED_UPDATE_INCOMPATIBLE, and with + # allowBackup false an uninstall destroys the tester's imported beacons), and it makes the + # Maps SHA-1 restriction impossible to satisfy, which is why maps rendered blank here and + # not locally. + # + # Absent secret is not fatal: a fork without it falls back to the generated key and still + # builds, which is the behaviour a fork wants. + # + # Passed through `env` rather than interpolated into the script: the `secrets` context is + # not available in a step-level `if` at all, and a `${{ }}` inside a run block puts the + # value on the command line, where a shell trace or an injected newline can expose it. + - name: Write the shared debug keystore + env: + DEBUG_KEYSTORE_BASE64: ${{ secrets.DEBUG_KEYSTORE_BASE64 }} + run: | + if [ -z "$DEBUG_KEYSTORE_BASE64" ]; then + echo "No DEBUG_KEYSTORE_BASE64 secret; AGP will generate a debug key for this run." + echo "APKs from this run cannot be installed over ones from another." + exit 0 + fi + printf '%s' "$DEBUG_KEYSTORE_BASE64" | base64 -d > app/debug-keystore.jks + echo "Debug keystore written, $(wc -c < app/debug-keystore.jks) bytes" + # KVM is required for a hardware-accelerated emulator; without it the run times out. - name: Enable KVM run: | @@ -197,6 +223,32 @@ jobs: run: | echo "MAPS_API_KEY=${{ secrets.MAPS_API_KEY || 'maps_key_default_value' }}" > secrets.properties + # **So every debug APK this repository publishes is signed by the same key.** + # + # A runner has no ~/.android/debug.keystore, so AGP generates one per run. That makes each + # debug APK unupgradeable over the last (INSTALL_FAILED_UPDATE_INCOMPATIBLE, and with + # allowBackup false an uninstall destroys the tester's imported beacons), and it makes the + # Maps SHA-1 restriction impossible to satisfy, which is why maps rendered blank here and + # not locally. + # + # Absent secret is not fatal: a fork without it falls back to the generated key and still + # builds, which is the behaviour a fork wants. + # + # Passed through `env` rather than interpolated into the script: the `secrets` context is + # not available in a step-level `if` at all, and a `${{ }}` inside a run block puts the + # value on the command line, where a shell trace or an injected newline can expose it. + - name: Write the shared debug keystore + env: + DEBUG_KEYSTORE_BASE64: ${{ secrets.DEBUG_KEYSTORE_BASE64 }} + run: | + if [ -z "$DEBUG_KEYSTORE_BASE64" ]; then + echo "No DEBUG_KEYSTORE_BASE64 secret; AGP will generate a debug key for this run." + echo "APKs from this run cannot be installed over ones from another." + exit 0 + fi + printf '%s' "$DEBUG_KEYSTORE_BASE64" | base64 -d > app/debug-keystore.jks + echo "Debug keystore written, $(wc -c < app/debug-keystore.jks) bytes" + # No stub wheel to validate any more: it is generated from app/stubs/unicorn/ by # generateUnicornStubWheel during the build, rather than being checked in. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2dd4ddf4..e7c0ad06 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -115,6 +115,46 @@ than replacing it. **Never uninstall a production install to force an install** `allowBackup` is false, so the beacons and location history are gone for good, and getting them back means redoing the macOS export. +### The shared debug keystore + +**Android signs debug builds with `~/.android/debug.keystore`, and a CI runner has no such file, +so it generates one per run.** Two things follow, and both were met before this was fixed: + +- **A debug APK cannot be installed over one from a different machine or a different CI run.** + Same `applicationId`, different signing key, so the install fails with + `INSTALL_FAILED_UPDATE_INCOMPATIBLE`. The only way forward is an uninstall, and an uninstall + destroys that device's imported beacons and location history. +- **Google Maps renders blank.** A Maps key is restricted by package name *and* signing SHA-1, + and a SHA-1 that changes every build cannot be whitelisted at all. It looks exactly like a + build with no API key in it. + +So CI writes one fixed keystore from the `DEBUG_KEYSTORE_BASE64` repository secret, and +`app/build.gradle.kts` uses `app/debug-keystore.jks` when that file is present. Its SHA-1 is +whitelisted against the Maps key, so **CI debug builds render maps and upgrade in place**. + +To make your local builds interchangeable with CI's, put the same keystore at +`app/debug-keystore.jks`: + +```bash +gh secret list -R parawanderer/OpenTagViewer # confirms it exists; secrets cannot be read back +# Ask a maintainer for the file, then: +ls -l app/debug-keystore.jks +keytool -list -v -keystore app/debug-keystore.jks -storepass android -alias androiddebugkey \ + | grep SHA1 +``` + +`*.jks` is gitignored, and the passwords are Android's well-known debug constants +(`android` / `androiddebugkey`) deliberately: the key proves nothing and guards nothing, and +giving it real secrets would only add something else to supply before the project builds. + +**Without the file, everything still builds** — Gradle logs a line saying so and falls back to +the generated key. That is the right behaviour for a fork, and it is also why a missing keystore +does not announce itself as an error when maps later come up blank. + +**Changing the debug key means one more uninstall, once.** Anything already installed was signed +with the old per-run key, so the first build after this lands still refuses to install over it. +After that they upgrade in place. + --- ## Testing diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 39c6432a..e24b5d7c 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -186,6 +186,46 @@ android { keyAlias = System.getenv("KEY_ALIAS") keyPassword = System.getenv("KEY_PASSWORD") } + + // **A debug key that is the same key every time, when one is supplied.** + // + // Without this, AGP signs debug builds with `~/.android/debug.keystore`, which a fresh + // CI runner *generates on the spot*. Two consequences, both of which bit: + // + // 1. Every CI debug APK is signed by a different key, so installing a newer one over an + // older one fails with INSTALL_FAILED_UPDATE_INCOMPATIBLE. The only way forward is to + // uninstall, and `allowBackup` is false, so that permanently destroys the imported + // beacons and location history on that device. Testing successive builds meant + // wiping the app every time. + // 2. A Google Maps key is restricted by package name *and* signing SHA-1, so a key whose + // SHA-1 changes per build cannot be whitelisted at all. Maps rendered blank in every + // CI debug build, which reads as the API key being missing from the build. + // + // Supplied through the environment rather than committed: it is a low-value key, but a + // signing key in a public repository is a bad habit to start, and `.gitignore` covers + // the filename. CI writes it from the DEBUG_KEYSTORE_BASE64 secret; see + // CONTRIBUTING.md for using the same one locally, which is what makes a locally built + // APK and a CI one interchangeable on the same device. + // + // **Falls back to AGP's default when absent**, so a clone with no keystore still builds. + // The passwords are Android's well-known debug constants on purpose: this key proves + // nothing and guards nothing, and inventing secrets for it would only mean another thing + // that has to be supplied before the project compiles. + getByName("debug") { + val supplied = file(System.getenv("DEBUG_KEYSTORE_FILE") ?: "debug-keystore.jks") + if (supplied.exists()) { + storeFile = supplied + storePassword = "android" + keyAlias = "androiddebugkey" + keyPassword = "android" + } else { + logger.lifecycle( + "No debug keystore at ${supplied.path}; using the default one. Debug APKs " + + "from this build will not match CI's, so installing one over the other " + + "needs an uninstall. See CONTRIBUTING.md." + ) + } + } } buildTypes { From 1bc2cf526c97c72fbd9a6194239ef55888421742 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:23:27 +0200 Subject: [PATCH 04/11] Pass an iCloud import back through Settings, so the map rebuilds Importing tags from the account via Settings left them invisible on the map, and showing in the device list as "No last location known", until the app was closed and reopened. Nothing was wrong with the data - the map reads its tags once, when it is created, and holds them in memory. FetchFromICloudActivity has always announced this. It sets RESULT_IMPORTED on the way out, and both the map and the device list act on it by rebuilding when they start that screen themselves. Settings started it with startActivity, which discards the result, so the one route people actually take to reconnect an account was the one route that dropped the signal. Settings now launches it for a result and carries the flag onward under the same key, so the map reads one name whatever screen it was reached through. The linked/unlinked subtitle is refreshed at the same time; it is read when Settings is built, so it said "not connected" under an account that had just been connected. Two tests: the flag survives the trip, and backing out does not claim an import - a result that always says "imported" costs a full rebuild and a refetch of every tag each time somebody opens that row and changes their mind. Not compiled here; this machine has no Android toolchain. Co-Authored-By: Claude Opus 5 (1M context) --- .../settings/FetchFromAccountSettingTest.java | 75 +++++++++++++++++++ .../android/opentagviewer/MapsActivity.java | 16 +++- .../opentagviewer/SettingsActivity.java | 47 +++++++++++- 3 files changed, 132 insertions(+), 6 deletions(-) diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java index 61b09a7e..5ed59e27 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java @@ -12,6 +12,10 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText; import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; import static org.hamcrest.Matchers.allOf; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import android.app.Activity; import android.app.Instrumentation.ActivityResult; @@ -152,4 +156,75 @@ public void tappingItReachesTheAccountScreen() { Eventually.check(() -> intended(hasComponent(FetchFromICloudActivity.class.getName()))); } + + /** + * And what that screen brought back is passed on, so the map rebuilds. + * + *

The map reads its tags once, when it is created, and holds them in memory. Reaching the + * account flow from here goes map, settings, iCloud - so coming back resumes the map instead + * of recreating it, and freshly imported tags were absent from it entirely. They showed in + * the device list as "No last location known", which reads as a fetch that failed rather + * than a screen that never learned they exist. Closing and reopening the app fixed it, which + * is the giveaway that nothing was wrong with the data. + * + *

{@code FetchFromICloudActivity} always set {@link FetchFromICloudActivity#RESULT_IMPORTED}; + * the map and the device list both act on it when they start that screen themselves. This + * screen used {@code startActivity}, which discards the result, so the one route through + * Settings was the one route that dropped it. + */ + @Test + public void whatTheAccountScreenImportedIsPassedBackToWhoeverOpenedSettings() { + final android.content.Intent imported = new android.content.Intent(); + imported.putExtra(FetchFromICloudActivity.RESULT_IMPORTED, true); + intending(hasComponent(FetchFromICloudActivity.class.getName())) + .respondWith(new ActivityResult(Activity.RESULT_OK, imported)); + + this.openSettings(); + + Eventually.check(() -> onView(withId(R.id.settings_fetch_from_account)) + .check(matches(isDisplayed()))); + onView(withId(R.id.settings_fetch_from_account)).perform(click()); + Eventually.check(() -> intended(hasComponent(FetchFromICloudActivity.class.getName()))); + + // Settings has to end for its own result to be readable, the same way the map ends it. + this.scenario.onActivity(Activity::finish); + + final androidx.test.core.app.ActivityScenario.Result result = + this.scenario.getResult(); + + assertEquals("Settings must report OK so the map looks at the data at all", + Activity.RESULT_OK, result.getResultCode()); + assertNotNull("nothing came back, so the map has nothing to act on", + result.getResultData()); + assertTrue("the import was not passed on, so the map never rebuilds and the tags stay" + + " invisible until the app is restarted", + result.getResultData() + .getBooleanExtra(FetchFromICloudActivity.RESULT_IMPORTED, false)); + } + + /** + * And it is not claimed when nothing was imported. + * + *

A result that always says "imported" costs a full rebuild of the map every time + * somebody opens this row and backs out, which is a visible flash and a refetch of every + * tag. The stub in {@link #answerTheFetchScreenAtTheDoor} cancels, which is what backing out + * of that screen does. + */ + @Test + public void backingOutOfItDoesNotClaimAnImport() { + this.openSettings(); + + Eventually.check(() -> onView(withId(R.id.settings_fetch_from_account)) + .check(matches(isDisplayed()))); + onView(withId(R.id.settings_fetch_from_account)).perform(click()); + Eventually.check(() -> intended(hasComponent(FetchFromICloudActivity.class.getName()))); + + this.scenario.onActivity(Activity::finish); + + final android.content.Intent data = this.scenario.getResult().getResultData(); + if (data != null) { + assertFalse("a cancelled account screen was reported as an import", + data.getBooleanExtra(FetchFromICloudActivity.RESULT_IMPORTED, false)); + } + } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java index 25617335..0d740daf 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java @@ -395,11 +395,19 @@ public void setZIndex(String markerId, float zIndex) { this.handleSendToLogin(); return; } - // Both want the same thing - a full rebuild. The tags live in memory here - // and that model is what decides both what is drawn and what is fetched, - // so showing or hiding the owner's devices is not a redraw. + // All three want the same thing - a full rebuild. The tags live in memory + // here and that model is what decides both what is drawn and what is + // fetched, so showing or hiding the owner's devices is not a redraw. + // + // The third is an iCloud import started from Settings. Reached from the map + // or the device list, that screen's result comes straight back and is acted + // on; reached through Settings it was dropped, so freshly imported tags were + // missing from the map and sat in the device list reading "No last location + // known" until the app was closed and reopened. if (data != null && (data.getBooleanExtra("mapProviderChanged", false) - || data.getBooleanExtra("shownDevicesChanged", false))) { + || data.getBooleanExtra("shownDevicesChanged", false) + || data.getBooleanExtra( + FetchFromICloudActivity.RESULT_IMPORTED, false))) { this.recreate(); } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/SettingsActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/SettingsActivity.java index e1b16d20..e2363efd 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/SettingsActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/SettingsActivity.java @@ -29,6 +29,7 @@ import com.google.android.material.slider.Slider; import android.widget.Toast; +import androidx.activity.result.ActivityResult; import androidx.activity.result.ActivityResultLauncher; import androidx.activity.result.contract.ActivityResultContracts; import androidx.appcompat.app.AlertDialog; @@ -136,6 +137,43 @@ public class SettingsActivity extends AppCompatActivity { */ private boolean shownDevicesChanged = false; + /** + * Whether connecting an iCloud account from here actually brought tags in. + * + *

Reported onward for the same reason as the two above, and it was not. The map + * holds its tags in memory and reads them once, when it is created. Reaching the account + * flow from here goes Map to Settings to iCloud, so returning resumes the map rather than + * recreating it, and tags that were just imported are absent until the app is closed and + * reopened. They sat in the device list reading "No last location known", which looks like + * a fetch that failed rather than a screen that never learned they exist. + * + *

{@code FetchFromICloudActivity} has always said so - it sets + * {@link FetchFromICloudActivity#RESULT_IMPORTED} on the way out, and both the map and the + * device list act on it when they launch that screen themselves. This screen started it with + * {@code startActivity}, which discards the result, so the one path through Settings was the + * one path that dropped the signal. + */ + private boolean importedFromAccount = false; + + /** + * Connecting an iCloud account, started from the row on this screen. + * + *

For a result, not fire-and-forget: see {@link #importedFromAccount}. + */ + private final ActivityResultLauncher fetchFromICloudLauncher = registerForActivityResult( + new ActivityResultContracts.StartActivityForResult(), + (ActivityResult result) -> { + final Intent data = result.getData(); + if (data != null + && data.getBooleanExtra(FetchFromICloudActivity.RESULT_IMPORTED, false)) { + this.importedFromAccount = true; + } + // The linked/unlinked subtitle is read when this screen is built, so without + // this it still says "not connected" underneath an account just connected. + this.sayWhetherTheAccountIsLinked(); + } + ); + /** * Where "help build full support" goes. * @@ -243,10 +281,13 @@ protected void onCreate(Bundle savedInstanceState) { } private void handleEndActivity() { - if (this.mapProviderChanged || this.shownDevicesChanged) { + if (this.mapProviderChanged || this.shownDevicesChanged || this.importedFromAccount) { Intent data = new Intent(); data.putExtra("mapProviderChanged", this.mapProviderChanged); data.putExtra("shownDevicesChanged", this.shownDevicesChanged); + // Carried under the name the iCloud screen uses, so the map reads one key whether + // that screen was reached from the map, the device list, or through here. + data.putExtra(FetchFromICloudActivity.RESULT_IMPORTED, this.importedFromAccount); setResult(RESULT_OK, data); } this.finish(); @@ -539,7 +580,9 @@ protected void onDestroy() { } private void onClickFetchFromAccount() { - this.startActivity(new Intent(this, FetchFromICloudActivity.class)); + // Launched for a result rather than with startActivity: what comes back decides whether + // the map has to rebuild. See importedFromAccount. + this.fetchFromICloudLauncher.launch(new Intent(this, FetchFromICloudActivity.class)); } private void onClickEditTheme() { From 478c9efd3d825ca36feee88a79395f784b5d25e7 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:27:28 +0200 Subject: [PATCH 05/11] Let the history sheet reach the bottom of the screen The per-tag history showed a band of the activity's white background below the grey sheet, and clipped the last row of the list above it. insetForSystemBars was applied to the activity root, which pads the coordinator inside it, so the sheet stopped short of the display edge. Its own documentation says not to use it on a screen that draws edge to edge - the map is named as the example - and a bottom sheet is exactly that case. So the bottom inset moves to the list, with clipToPadding=false so it is space the last row can scroll into rather than a dead band that clips it. The root keeps the status bar and the side insets, and the sheet's own background runs to the bottom of the screen again. Added as a paired overload taking both views rather than a top-only helper. A top-only one already existed once and was deleted for cause: it was applied to seven screens and the matching bottom call to one, which is how buttons ended up unreachable behind the gesture pill. Passing both leaves nowhere to put the omission. Six tests, dispatching the insets rather than waiting for them - the values that break this are not the ones a test device reports, and a dispatched inset is the only way to assert what a *second* delivery does. They pin both failures that have shipped: padding that stacked on every rotation, and a root-padded screen whose sheet stopped short. Not compiled here; this machine has no Android toolchain. Co-Authored-By: Claude Opus 5 (1M context) --- .../ui/compat/WindowPaddingUtilTest.java | 158 ++++++++++++++++++ .../opentagviewer/HistoryViewActivity.java | 9 +- .../ui/compat/WindowPaddingUtil.java | 54 ++++++ .../res/layout/view_history_bottom_sheet.xml | 8 +- 4 files changed, 227 insertions(+), 2 deletions(-) create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtilTest.java diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtilTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtilTest.java new file mode 100644 index 00000000..f1579b19 --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtilTest.java @@ -0,0 +1,158 @@ +package dev.wander.android.opentagviewer.ui.compat; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.junit.Assert.assertEquals; + +import android.content.Context; +import android.view.View; +import android.widget.FrameLayout; + +import androidx.core.graphics.Insets; +import androidx.core.view.ViewCompat; +import androidx.core.view.WindowInsetsCompat; +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * Where the system-bar insets land. + * + *

This is a screenshot bug that no screenshot test catches, because the insets a test + * device reports are not the ones that break it: a gesture-navigation phone has a bottom inset of + * a few dp, and three-button navigation has around 48. So the values are dispatched here rather + * than waited for, which is also the only way to assert what happens on a *second* delivery. + * + *

Both failures being pinned have shipped. Padding applied twice grew the gap on every + * rotation; padding the root of a screen with a bottom sheet left the sheet stopping short of the + * screen edge, showing a band of the activity's background beneath it and clipping the last row + * of the list. + */ +@RunWith(AndroidJUnit4.class) +public class WindowPaddingUtilTest { + + private static final int STATUS_BAR = 60; + private static final int NAV_BAR = 48; + + private Context context; + + @Before + public void setUp() { + this.context = getInstrumentation().getTargetContext(); + } + + private static WindowInsetsCompat systemBars() { + return new WindowInsetsCompat.Builder() + .setInsets( + WindowInsetsCompat.Type.systemBars(), + Insets.of(0, STATUS_BAR, 0, NAV_BAR)) + .build(); + } + + /** Insets arrive on their own schedule, so a test has to hand them over itself. */ + private static void deliverInsetsTo(final View view) { + ViewCompat.dispatchApplyWindowInsets(view, systemBars()); + } + + @Test + public void awholeScreenIsKeptClearOfBothBars() { + final View screen = new FrameLayout(this.context); + + WindowPaddingUtil.insetForSystemBars(screen); + deliverInsetsTo(screen); + + assertEquals("the status bar would cover the heading", STATUS_BAR, screen.getPaddingTop()); + assertEquals("a button here would be behind the navigation bar", + NAV_BAR, screen.getPaddingBottom()); + } + + /** + * And the padding a layout already asked for is kept rather than replaced. + */ + @Test + public void bitaddsToThePaddingTheLayoutAlreadyHad() { + final View screen = new FrameLayout(this.context); + screen.setPadding(0, 7, 0, 11); + + WindowPaddingUtil.insetForSystemBars(screen); + deliverInsetsTo(screen); + + assertEquals(STATUS_BAR + 7, screen.getPaddingTop()); + assertEquals(NAV_BAR + 11, screen.getPaddingBottom()); + } + + /** + * Delivered twice, the gap does not double. + * + *

Insets arrive more than once - a rotation, a keyboard, switching to three-button + * navigation - and reading the view's current padding inside the listener would add to a + * value that already includes the last delivery. + */ + @Test + public void ctwodeliveriesDoNotStack() { + final View screen = new FrameLayout(this.context); + screen.setPadding(0, 7, 0, 11); + + WindowPaddingUtil.insetForSystemBars(screen); + deliverInsetsTo(screen); + deliverInsetsTo(screen); + + assertEquals(STATUS_BAR + 7, screen.getPaddingTop()); + assertEquals(NAV_BAR + 11, screen.getPaddingBottom()); + } + + /** + * The paired form puts the bottom on the content and leaves the root's alone. + * + *

The history screen's sheet has to reach the bottom of the display. Padding its root + * instead shortened everything inside it, so the sheet stopped above the navigation bar with + * the activity's background showing beneath it - a white band under a grey sheet - and the + * last row of the list clipped by the same gap. + */ + @Test + public void dthepairedFormGivesTheBottomToTheContent() { + final View root = new FrameLayout(this.context); + final View list = new FrameLayout(this.context); + + WindowPaddingUtil.insetForSystemBars(root, list); + deliverInsetsTo(root); + + assertEquals("the root still clears the status bar", STATUS_BAR, root.getPaddingTop()); + assertEquals("the root must reach the bottom edge, or the sheet stops short", + 0, root.getPaddingBottom()); + assertEquals("the list has to clear the navigation bar itself", + NAV_BAR, list.getPaddingBottom()); + } + + /** The content's own padding survives, and the root's does too. */ + @Test + public void ethepairedFormKeepsBothViewsOwnPadding() { + final View root = new FrameLayout(this.context); + final View list = new FrameLayout(this.context); + root.setPadding(0, 3, 0, 5); + list.setPadding(0, 0, 0, 9); + + WindowPaddingUtil.insetForSystemBars(root, list); + deliverInsetsTo(root); + + assertEquals(STATUS_BAR + 3, root.getPaddingTop()); + assertEquals("the root's own bottom padding is not the navigation bar's, and stays", + 5, root.getPaddingBottom()); + assertEquals(NAV_BAR + 9, list.getPaddingBottom()); + } + + /** And it does not stack on a second delivery either. */ + @Test + public void fthepairedFormDoesNotStackOnTheContent() { + final View root = new FrameLayout(this.context); + final View list = new FrameLayout(this.context); + list.setPadding(0, 0, 0, 9); + + WindowPaddingUtil.insetForSystemBars(root, list); + deliverInsetsTo(root); + deliverInsetsTo(root); + + assertEquals(NAV_BAR + 9, list.getPaddingBottom()); + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/HistoryViewActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/HistoryViewActivity.java index 62c027e4..f078b0d4 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/HistoryViewActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/HistoryViewActivity.java @@ -180,7 +180,6 @@ protected void onCreate(Bundle savedInstanceState) { .blockingFirst(); ActivityHistoryViewBinding binding = DataBindingUtil.setContentView(this, R.layout.activity_history_view); - WindowPaddingUtil.insetForSystemBars(binding.getRoot()); binding.setHandleClickBack(this::finish); binding.setPageTitle(this.getCurrentBeaconName()); @@ -202,6 +201,14 @@ protected void onCreate(Bundle savedInstanceState) { this::handleOnClickHistoryListItem ); RecyclerView recyclerView = findViewById(R.id.recycler_view_history_items); + + // **The bottom inset goes on the list, not on the root.** Padding the root shortens the + // coordinator inside it, so the sheet stopped above the navigation bar and the white + // activity background showed through beneath a grey sheet - with the last history row + // clipped by the same gap. Paired call, so the bottom cannot be left out: see + // WindowPaddingUtil. + WindowPaddingUtil.insetForSystemBars(binding.getRoot(), recyclerView); + recyclerView.setLayoutManager(new LinearLayoutManager(this)); recyclerView.setAdapter(this.historyItemsAdapter); recyclerView.setItemAnimator(null); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtil.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtil.java index e0ca3536..9f89aebf 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtil.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/compat/WindowPaddingUtil.java @@ -56,6 +56,60 @@ public static void insetForSystemBars(final View view) { }); } + /** + * For a screen whose bottom edge belongs to something that has to reach it. + * + *

A bottom sheet is the case this exists for. Padding the root's bottom shortens + * everything inside it, so the sheet stops above the navigation bar and the activity's own + * background shows through underneath - a strip in the window's colour, below a sheet in the + * sheet's colour, which reads as a rendering fault rather than as padding. The history + * screen shipped that way: a grey sheet, its last row clipped, and a white band under it. + * + *

Both views are arguments because one of them always gets forgotten otherwise. + * That is not hypothetical - see {@link #insetForSystemBars(View)}, which exists in that + * shape because a top-only helper was applied to seven screens and the matching bottom call + * to one. A caller here cannot pad the top and quietly skip the bottom: there is nowhere to + * put the omission. + * + *

{@code content} is padded rather than the sheet itself, so the sheet's background still + * runs to the bottom of the screen. Give it {@code clipToPadding="false"} when it scrolls, + * or the padding becomes a dead band the list cannot use instead of somewhere the last row + * can scroll into. + * + * @param root Gets the status bar, and the left and right insets. Not the bottom. + * @param content Gets the bottom inset, added to whatever padding it already asks for. + */ + public static void insetForSystemBars(final View root, final View content) { + final int rootLeft = root.getPaddingLeft(); + final int rootTop = root.getPaddingTop(); + final int rootRight = root.getPaddingRight(); + final int rootBottom = root.getPaddingBottom(); + + final int contentLeft = content.getPaddingLeft(); + final int contentTop = content.getPaddingTop(); + final int contentRight = content.getPaddingRight(); + final int contentBottom = content.getPaddingBottom(); + + ViewCompat.setOnApplyWindowInsetsListener(root, (v, insets) -> { + final Insets bars = insets.getInsets(WindowInsetsCompat.Type.systemBars()); + + v.setPadding( + rootLeft + bars.left, + rootTop + bars.top, + rootRight + bars.right, + rootBottom + ); + content.setPadding( + contentLeft, + contentTop, + contentRight, + contentBottom + bars.bottom + ); + + return insets; + }); + } + /** * Keeps a bottom-anchored view clear of the navigation bar. * diff --git a/app/src/main/res/layout/view_history_bottom_sheet.xml b/app/src/main/res/layout/view_history_bottom_sheet.xml index b043b9ac..9a2d4d8d 100644 --- a/app/src/main/res/layout/view_history_bottom_sheet.xml +++ b/app/src/main/res/layout/view_history_bottom_sheet.xml @@ -158,10 +158,16 @@ + + android:layout_height="match_parent" + android:clipToPadding="false" /> From 52357a0c25d3f44eca5feb6f102a5d92abaed1d6 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:29:22 +0200 Subject: [PATCH 06/11] Fix the settings-result test's use of ActivityScenario Two mistakes, one caught by CI and one caught reading the API afterwards. ActivityScenario.Result does not exist; getResult() answers with Instrumentation.ActivityResult, which this file already imports for the stub. That was the compile error. And getResult() throws unless the scenario was created with launchActivityForResult - the ordinary launch() compiles against it perfectly happily and fails at run time, so the emulator would have gone red a second time for a different reason. Both are the cost of writing an instrumented test on a machine with no Android toolchain. Stated rather than hidden: the rest of this branch's Java is unverified here in the same way. Co-Authored-By: Claude Opus 5 (1M context) --- .../settings/FetchFromAccountSettingTest.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java index 5ed59e27..285703dd 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java @@ -93,6 +93,17 @@ private void openSettings() { this.scenario = ActivityScenario.launch(SettingsActivity.class); } + /** + * The same screen, launched so that {@code getResult()} is allowed to answer. + * + *

{@code ActivityScenario.getResult()} throws unless the scenario was created with + * {@code launchActivityForResult}, which is not a detail that shows up until it runs - the + * ordinary {@code launch} compiles against it perfectly happily. + */ + private void openSettingsExpectingAResult() { + this.scenario = ActivityScenario.launchActivityForResult(SettingsActivity.class); + } + /** As if the app had already joined the account's keychain. */ private void givenTheAccountIsAlreadyLinked() { this.memberships.store(new KeychainMembership( @@ -179,7 +190,7 @@ public void whatTheAccountScreenImportedIsPassedBackToWhoeverOpenedSettings() { intending(hasComponent(FetchFromICloudActivity.class.getName())) .respondWith(new ActivityResult(Activity.RESULT_OK, imported)); - this.openSettings(); + this.openSettingsExpectingAResult(); Eventually.check(() -> onView(withId(R.id.settings_fetch_from_account)) .check(matches(isDisplayed()))); @@ -189,8 +200,9 @@ public void whatTheAccountScreenImportedIsPassedBackToWhoeverOpenedSettings() { // Settings has to end for its own result to be readable, the same way the map ends it. this.scenario.onActivity(Activity::finish); - final androidx.test.core.app.ActivityScenario.Result result = - this.scenario.getResult(); + // ActivityScenario.getResult() hands back Instrumentation.ActivityResult, the same type + // the stub above is built from. + final ActivityResult result = this.scenario.getResult(); assertEquals("Settings must report OK so the map looks at the data at all", Activity.RESULT_OK, result.getResultCode()); @@ -212,7 +224,7 @@ public void whatTheAccountScreenImportedIsPassedBackToWhoeverOpenedSettings() { */ @Test public void backingOutOfItDoesNotClaimAnImport() { - this.openSettings(); + this.openSettingsExpectingAResult(); Eventually.check(() -> onView(withId(R.id.settings_fetch_from_account)) .check(matches(isDisplayed()))); From fb21cd46dc5fda24ca8300ef7ee7c974214a927f Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:37:31 +0200 Subject: [PATCH 07/11] Say which battery reading is which, and why two decoders disagree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are two battery values in this app and the documentation described only one of them, so a question about the badge on the map was answered from the wrong file - by me, confidently, before checking. BatteryLevelDescription covers the accessory record's field, written by Apple's own devices, and said "nothing outside the debug panel uses any of it". The map's tag cards have shown "Nearby · Battery ..." since the BLE scan landed, and that comes from the tag's own advertisement through FindMyAdvertisement.BatteryLevel - a different source, a different scale (0-3 against this field's 1-4, which reserves 0 for "not reported"), and a different age. They disagree routinely and both can be right. The two decoders also looked contradictory. LocationReportFields refuses to read the status byte unless it conforms to Apple's Table 5-5, and argues at length that decoding an AirTag's 0x90 against that table yields "Low" for a tag whose record says Full. FindMyAdvertisement reads bits 6-7 with no gate at all. That difference is defensible and was undocumented: only the two bits are used and not the rest of the table, so the reserved bits an AirTag sets wrongly are the ones nothing looks at; the one published observation of a real AirTag (0x10, bits 6-7 = full) agrees; and for a user with no Apple device the advertisement is the only source there is. Now written down in all three files, along with the part that matters most - nobody has checked either mapping against a tag at a known charge level, so a reading that disagrees with a fresh battery is as likely to be the decoder as the cell. Comments only. Co-Authored-By: Claude Opus 5 (1M context) --- .../ble/FindMyAdvertisement.java | 28 +++++++++++++++++++ .../util/parse/BatteryLevelDescription.java | 17 +++++++++-- .../util/parse/LocationReportFields.java | 9 ++++++ 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java b/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java index 0f44c48b..5a64f390 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java @@ -92,6 +92,34 @@ public static FindMyAdvertisement parse(@Nullable final byte[] appleManufacturer return new FindMyAdvertisement(state, batteryLevelOf(status), status); } + /** + * Bits 6-7 of the status byte, read as a battery level. + * + *

Read on sight, unlike the same byte in a location report. + * {@code LocationReportFields} decodes its copy only when the whole byte conforms to Apple's + * Table 5-5 - bit 5 set, reserved bits clear - because an AirTag's does not, and decoding a + * non-conforming byte against that table produces a confident wrong answer. That gate is not + * applied here, and the difference is deliberate rather than an oversight: + * + *

    + *
  • Only these two bits are used, not the rest of the table. The reserved bits an + * AirTag sets wrongly are the ones this does not look at.
    • + *
  • The one published observation of a real AirTag agrees. Adam Catley's teardown + * records {@code 0x10}, whose bits 6-7 are {@code 0b00} - "full", for a working tag. + * Consistent, if only just: one data point at one battery level.
    • + *
  • There is no alternative for these users. The battery on the account record is + * written by Apple's own devices, so for somebody without one it is years old or, as + * with both tags this was developed against, never written at all. See + * {@code LastBleSighting}.
    • + *
+ * + *

So this is what the tag claimed, not a measurement, and nobody has checked the + * mapping against tags at known levels. That is the experiment worth doing: sit down with + * several accessories at several charge levels and write down what each one emits. Until + * somebody has, a reading here that disagrees with a fresh battery is as likely to be this + * decoder as the cell - which is exactly how it was queried. The raw byte is kept on the + * advertisement so a bug report can quote it instead of only this reading. + */ private static BatteryLevel batteryLevelOf(final int statusByte) { switch ((statusByte >> 6) & 0b11) { case 0b01: return BatteryLevel.MEDIUM; diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/BatteryLevelDescription.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/BatteryLevelDescription.java index f66488a1..5eecb6fd 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/BatteryLevelDescription.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/BatteryLevelDescription.java @@ -49,9 +49,20 @@ *

It only means anything for a tag read from an Apple account. The field is updated by * Apple's own devices as they see the accessory, so a tag imported from a zip carries whatever * value was true when the export was made and never changes it again - possibly years ago. This - * is why nothing outside the debug panel uses any of it. Anyone who wants to put a battery icon - * on the device list should read this note first, and should probably only do it for account - * tags. + * is why nothing outside the device information page uses any of it. Anyone who wants to put a + * battery icon on the device list should read this note first, and should probably only do it + * for account tags. + * + *

There is a second battery reading in this app, and it is not this one. The badge on + * the map's tag cards - "Nearby · Battery …" - comes from the tag's own Bluetooth advertisement + * via {@code FindMyAdvertisement.BatteryLevel}, heard directly by this phone, and it has nothing + * to do with this field or this scale. They disagree routinely and both can be right: this one + * is what Apple last recorded, that one is what the tag said just now. Somebody reading the + * screen sees one word and no indication of which. + * + *

Worth knowing before answering a question about either. The values are not comparable - + * this field reserves 0 for "not reported" and so runs 1-4, while the advertisement's two bits + * run 0-3 - and neither has been validated against a tag at a known charge level. */ @NoArgsConstructor(access = AccessLevel.PRIVATE) public final class BatteryLevelDescription { diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java index 669c8381..01aacac4 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java @@ -114,6 +114,15 @@ * with several kinds of tag at several battery levels and writing down what each one emits. The * gate below exists so that the app stays useful and silent in the meantime, instead of guessing. * + *

The live Bluetooth path does read two of these bits, and that is not a contradiction of + * the paragraph above. {@code FindMyAdvertisement.batteryLevelOf} takes bits 6-7 off an + * advertisement this phone heard itself, and never consults the rest of the table - so the + * reserved bits an AirTag sets wrongly are the ones it does not look at. It also has no + * alternative: the battery on the account record is written by Apple's devices, so for a user + * without one it is years old or never written. What is refused here is decoding a + * non-conforming byte as though the whole table applied, which is a different claim. + * Neither reading has been checked against a tag at a known charge level. + * *

So {@link #status(long)} decodes only a byte that actually conforms to Table 5-5 - bit 5 set * and every reserved bit clear - and otherwise shows the number alone. A conforming byte is * annotated as what the beacon claimed, never as a measurement. Every value carries decimal, From 4f9b247230d5f8d53d752c4c99e15689823eb925 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:39:03 +0200 Subject: [PATCH 08/11] Record two real AirTag status bytes, which settle the battery bits @parawanderer read the history of two tags on one account on 2026-09-16: 0x10 = 0b00010000 on the tag reporting full, 0x50 = 0b01010000 on the tag reporting medium. The useful part is what does not change. The two bytes are identical in every bit except 6 and 7 - including bit 4 set and bit 5 clear, the two that break Apple's Table 5-5 and are the whole reason LocationReportFields refuses to decode this byte. A remainder that is constant across two tags in different battery states is a signature, not a field, so the objection does not reach bits 6-7, and those move with the battery in the order the table gives. Catley's teardown independently records 0x10 on a working tag. So the decoder in FindMyAdvertisement is now evidenced rather than merely defensible, and both files say so. It settles less than it looks like, and that is written down too: the bits are what the accessory claims about itself. Nothing here calibrates the four words against actual charge, and a tag samples its cell on its own schedule, so a freshly replaced battery reporting the old level for a while is expected rather than a decoding fault. That was the question that started this. Comments only. Co-Authored-By: Claude Opus 5 (1M context) --- .../ble/FindMyAdvertisement.java | 32 +++++++++++++------ .../util/parse/LocationReportFields.java | 8 ++++- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java b/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java index 5a64f390..622af8ce 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java @@ -104,21 +104,35 @@ public static FindMyAdvertisement parse(@Nullable final byte[] appleManufacturer *

    *
  • Only these two bits are used, not the rest of the table. The reserved bits an * AirTag sets wrongly are the ones this does not look at.
    • - *
  • The one published observation of a real AirTag agrees. Adam Catley's teardown - * records {@code 0x10}, whose bits 6-7 are {@code 0b00} - "full", for a working tag. - * Consistent, if only just: one data point at one battery level.
    • + *
  • Two real AirTags, side by side, differ in exactly these two bits and nowhere + * else. Observed on 2026-09-16 by @parawanderer, from the history of two tags on + * one account: {@code 0x10} = {@code 0b00010000} on the tag reading full, {@code 0x50} + * = {@code 0b01010000} on the tag reading medium. Bits 6-7 are {@code 0b00} and + * {@code 0b01}; every other bit is identical, including the two an AirTag sets against + * Table 5-5. + * + *

    That is the useful shape of the result. The non-conforming remainder is a + * constant - an AirTag signature, not a battery field - so the objection to decoding + * this byte does not reach bits 6-7, and the levels come out in the order Table 5-5 + * gives. Adam Catley's teardown independently records {@code 0x10} on a working + * tag.

    • *
  • There is no alternative for these users. The battery on the account record is * written by Apple's own devices, so for somebody without one it is years old or, as * with both tags this was developed against, never written at all. See * {@code LastBleSighting}.
    • *
* - *

So this is what the tag claimed, not a measurement, and nobody has checked the - * mapping against tags at known levels. That is the experiment worth doing: sit down with - * several accessories at several charge levels and write down what each one emits. Until - * somebody has, a reading here that disagrees with a fresh battery is as likely to be this - * decoder as the cell - which is exactly how it was queried. The raw byte is kept on the - * advertisement so a bug report can quote it instead of only this reading. + *

What that does not establish is whether the tag is right. These bits are what the + * accessory says about itself, and the observation above shows only that two tags in + * different states say different things in the expected order. It does not calibrate the + * words: nothing here knows what charge "medium" corresponds to, and a tag samples its cell + * on its own schedule, so a freshly replaced battery can keep reporting the old one's level + * for a while. A tag that reads medium on a new cell is therefore not evidence of a bug in + * this decoder - which is the question that produced this note. + * + *

Still wanted, and now a smaller job than it was: the same two bits read off tags whose + * actual charge is known, to attach numbers to the four words. The raw byte is kept on the + * advertisement so any such report can quote it rather than only this reading. */ private static BatteryLevel batteryLevelOf(final int statusByte) { switch ((statusByte >> 6) & 0b11) { diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java index 01aacac4..7a28914f 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java @@ -121,7 +121,13 @@ * alternative: the battery on the account record is written by Apple's devices, so for a user * without one it is years old or never written. What is refused here is decoding a * non-conforming byte as though the whole table applied, which is a different claim. - * Neither reading has been checked against a tag at a known charge level. + * + *

Two AirTags on one account, read on 2026-09-16, support the narrower reading: {@code 0x10} + * on the one reporting full and {@code 0x50} on the one reporting medium - identical in every + * bit except 6 and 7, including the two that break this table. So the non-conforming remainder + * looks like a fixed AirTag signature rather than a field, and bits 6-7 move with the battery in + * the order Table 5-5 gives. What no observation yet fixes is what the four words mean in charge + * terms, or whether the tag re-measures promptly after a cell is changed. * *

So {@link #status(long)} decodes only a byte that actually conforms to Table 5-5 - bit 5 set * and every reserved bit clear - and otherwise shows the number alone. A conforming byte is From cf7bf4b33d60713fdd088b910b87f91585992351 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:41:20 +0200 Subject: [PATCH 09/11] Correct the 0x90 claim: the advertisement was right, the record was stale LocationReportFields argued against decoding the accessory status byte, and its headline example was that decoding 0x90 gives "battery Low" for a tag whose own record reads Full - offered as proof the byte lies. It was the wrong way round. @parawanderer's two tags reported 0x90 for months on cells that had not been changed in as long, over both Bluetooth and the Find My network, and replacing the batteries moved them to 0x10 and 0x50. "Low" was correct. The stale value was the accessory record saying Full: that field is written by Apple's own devices, and this user has none, so it had never been written at all. The correction is left visible rather than quietly removed. The claim was written confidently, it was load-bearing for the decision it justified, and what disproved it was somebody changing two batteries and looking. Three of the four states are now recorded in both files, with the remainder 0b_0010000 constant across all three - bit 4 set, bit 5 clear, the two an AirTag sets against Table 5-5. A remainder that does not move across three battery states is a signature rather than a field, so the objection never reached bits 6-7, and those fall in the order the table gives: downward as a cell ages, upward when it is replaced. What is still unknown is narrower than before and said as such: nothing calibrates the four words against actual charge, and 0b11 has never been observed. Comments only. Co-Authored-By: Claude Opus 5 (1M context) --- .../ble/FindMyAdvertisement.java | 43 ++++++++++--------- .../util/parse/LocationReportFields.java | 34 +++++++++++---- 2 files changed, 48 insertions(+), 29 deletions(-) diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java b/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java index 622af8ce..aaef5c8d 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ble/FindMyAdvertisement.java @@ -104,35 +104,36 @@ public static FindMyAdvertisement parse(@Nullable final byte[] appleManufacturer *

    *
  • Only these two bits are used, not the rest of the table. The reserved bits an * AirTag sets wrongly are the ones this does not look at.
    • - *
  • Two real AirTags, side by side, differ in exactly these two bits and nowhere - * else. Observed on 2026-09-16 by @parawanderer, from the history of two tags on - * one account: {@code 0x10} = {@code 0b00010000} on the tag reading full, {@code 0x50} - * = {@code 0b01010000} on the tag reading medium. Bits 6-7 are {@code 0b00} and - * {@code 0b01}; every other bit is identical, including the two an AirTag sets against - * Table 5-5. + *
  • Two real AirTags, watched across a battery change, move in exactly these two bits + * and nowhere else. Observed by @parawanderer on 2026-09-16, over both Bluetooth + * and the Find My network: * - *

    That is the useful shape of the result. The non-conforming remainder is a - * constant - an AirTag signature, not a battery field - so the objection to decoding - * this byte does not reach bits 6-7, and the levels come out in the order Table 5-5 - * gives. Adam Catley's teardown independently records {@code 0x10} on a working - * tag.

    • + * 
    +     *   0x90 = 0b10010000   both tags, cells months old     bits 6-7 = 0b10  Low
+     *   0x50 = 0b01010000   one tag, cell just replaced     bits 6-7 = 0b01  Medium
+     *   0x10 = 0b00010000   the other, cell just replaced   bits 6-7 = 0b00  Full
+     *
    + * + *

    Three of the four states, with bit 4 set and bit 5 clear throughout - the two an + * AirTag sets against Table 5-5. A remainder that does not change across three battery + * states is a signature, not a field, so the objection to decoding this byte does not + * reach bits 6-7. They fall in the order the table gives, downward as a cell ages and + * upward when it is replaced. Catley's teardown independently records {@code 0x10}. *

  • There is no alternative for these users. The battery on the account record is * written by Apple's own devices, so for somebody without one it is years old or, as * with both tags this was developed against, never written at all. See * {@code LastBleSighting}.
    • *
* - *

What that does not establish is whether the tag is right. These bits are what the - * accessory says about itself, and the observation above shows only that two tags in - * different states say different things in the expected order. It does not calibrate the - * words: nothing here knows what charge "medium" corresponds to, and a tag samples its cell - * on its own schedule, so a freshly replaced battery can keep reporting the old one's level - * for a while. A tag that reads medium on a new cell is therefore not evidence of a bug in - * this decoder - which is the question that produced this note. + *

What it still does not establish is what the four words are worth. Nothing here + * calibrates them: "medium" on a cell replaced minutes earlier is the tag's own opinion, and + * whether that reflects a weak cell, a measurement the tag has not retaken, or a scale that + * simply does not start at "full" is unknown. Only {@code 0b11}, critically low, has not + * been seen at all. * - *

Still wanted, and now a smaller job than it was: the same two bits read off tags whose - * actual charge is known, to attach numbers to the four words. The raw byte is kept on the - * advertisement so any such report can quote it rather than only this reading. + *

Still wanted, and a much smaller job than before: these bits read off tags whose actual + * charge is known, to attach numbers to the words. The raw byte is kept on the advertisement + * so any such report can quote it rather than only this reading. */ private static BatteryLevel batteryLevelOf(final int statusByte) { switch ((statusByte >> 6) & 0b11) { diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java index 7a28914f..811197aa 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java @@ -97,8 +97,19 @@ * it observes is {@code 0x90}: bit 5 clear where the specification requires it set, and reserved * bit 4 set. Adam Catley's teardown records a real AirTag advertising {@code 0x10}, which breaks * the same two rules. The specification governs third-party MFi accessories; AirTag is Apple's own - * hardware and predates it. Decoding {@code 0x90} against Table 5-5 anyway yields "battery Low" - * for a tag whose own record reads Full - a confident, wrong answer, which is worse than none. + * hardware and predates it. + * + *

This paragraph used to end by saying that decoding {@code 0x90} yields "battery Low" for + * a tag whose record reads Full, and offering that as the proof that the byte lies. It was the + * wrong way round. @parawanderer's two tags read {@code 0x90} for months on cells that had + * not been changed in as long, over both Bluetooth and the Find My network; replacing the + * batteries moved them to {@code 0x10} and {@code 0x50}. "Low" was correct. What was stale was + * the accessory record saying Full - written by Apple's own devices, of which this user has + * none, so it had never been updated at all. + * + *

Worth keeping the correction visible rather than quietly deleting the claim: it was written + * confidently, it was load-bearing for the decision below, and the thing that disproved it was + * somebody changing two batteries and looking. * *

And the byte is not trustworthy even when it is well-formed. Caesar Creek Software's * write-up of this network puts it plainly: "it's supposed to indicate the battery level and @@ -122,12 +133,19 @@ * without one it is years old or never written. What is refused here is decoding a * non-conforming byte as though the whole table applied, which is a different claim. * - *

Two AirTags on one account, read on 2026-09-16, support the narrower reading: {@code 0x10} - * on the one reporting full and {@code 0x50} on the one reporting medium - identical in every - * bit except 6 and 7, including the two that break this table. So the non-conforming remainder - * looks like a fixed AirTag signature rather than a field, and bits 6-7 move with the battery in - * the order Table 5-5 gives. What no observation yet fixes is what the four words mean in charge - * terms, or whether the tag re-measures promptly after a cell is changed. + *

Two AirTags on one account, watched across a battery change on 2026-09-16, give three of + * the four states - and the same remainder every time: + * + * 

+ *   0x90 = 0b10010000   both tags, on cells months old        bits 6-7 = 0b10  Low
+ *   0x50 = 0b01010000   one tag, cell just replaced           bits 6-7 = 0b01  Medium
+ *   0x10 = 0b00010000   the other, cell just replaced         bits 6-7 = 0b00  Full
+ *
+ * + *

Bit 4 is set and bit 5 clear in all three - the two that break this table - while only bits + * 6-7 move, and they move in the order Table 5-5 gives, downward as the cell ages and upward + * when it is replaced. A remainder constant across three battery states is a signature rather + * than a field. Catley's teardown independently records {@code 0x10}. * *

So {@link #status(long)} decodes only a byte that actually conforms to Table 5-5 - bit 5 set * and every reserved bit clear - and otherwise shows the number alone. A conforming byte is From ada968246f835f4b8b6054b21f7fe6a19192a97f Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 16:43:04 +0200 Subject: [PATCH 10/11] Record why the status byte stays undecoded, now its old reason is gone The previous commit disproved the example that justified the conformance gate on the debug row's status byte. It did not change the decision, and a disproved justification with the decision still standing is exactly the shape a later agent removes as an oversight. So the actual reason is written down, and it does not depend on the protocol argument at all: asked directly, @parawanderer's answer was that this is debug metadata and does not need decoding. That row exists so somebody can quote what arrived; the raw byte is certainly right, and a label beside it would be this app's opinion competing with the tag's own on a screen meant for evidence. The battery reading people act on is on the map, from the live advertisement, as one word. Comments only. Last change to this branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../util/parse/LocationReportFields.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java index 811197aa..d849f3a8 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/LocationReportFields.java @@ -147,6 +147,18 @@ * when it is replaced. A remainder constant across three battery states is a signature rather * than a field. Catley's teardown independently records {@code 0x10}. * + *

The gate below stays anyway, and not because of the claim just corrected. Asked + * directly, on 2026-09-16, @parawanderer's answer was that this is debug metadata and does not + * need decoding. That is the reason to keep in mind, because it does not depend on any of the + * protocol argument above: this row exists so somebody can quote what arrived, the raw byte is + * certainly right, and a label beside it would be the app's opinion competing with the tag's + * own on a screen meant for evidence. The battery reading people act on is on the map, from the + * live advertisement, where it is one word and not a bit pattern. + * + *

Written down because the paragraph above removed a justification without removing the + * decision. Anybody reading "the 0x90 objection was wrong" and concluding that this should now + * decode is following an argument nobody made. + * *

So {@link #status(long)} decodes only a byte that actually conforms to Table 5-5 - bit 5 set * and every reserved bit clear - and otherwise shows the number alone. A conforming byte is * annotated as what the beacon claimed, never as a measurement. Every value carries decimal, From 94893b5514644121542a37a32dec5271dd2d272c Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Wed, 16 Sep 2026 17:02:50 +0200 Subject: [PATCH 11/11] Back out of Settings in the test, rather than calling finish() The emulator run failed on one test out of 747, and it was this one: expected RESULT_OK, got RESULT_CANCELED. finish() skips handleEndActivity(), which is the method that sets the result at all - so the test drove an exit path the app never takes and then reported that the flag had been dropped by a screen nobody asked to report one. The production code was right; the test was pulling the wrong lever. pressBackUnconditionally goes through onBackPressed and therefore through handleEndActivity, which is what a user does. Unconditionally because plain pressBack throws when the activity it finishes is the last one, which here it always is. Third correction to this test, all of them things javac or the device would have told me in seconds on a machine with an Android toolchain. Co-Authored-By: Claude Opus 5 (1M context) --- .../ui/settings/FetchFromAccountSettingTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java index 285703dd..d13d23c9 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/settings/FetchFromAccountSettingTest.java @@ -197,8 +197,11 @@ public void whatTheAccountScreenImportedIsPassedBackToWhoeverOpenedSettings() { onView(withId(R.id.settings_fetch_from_account)).perform(click()); Eventually.check(() -> intended(hasComponent(FetchFromICloudActivity.class.getName()))); - // Settings has to end for its own result to be readable, the same way the map ends it. - this.scenario.onActivity(Activity::finish); + // **Backed out, not finished.** Calling finish() directly skips handleEndActivity(), + // which is the method that sets the result at all - so the test reported RESULT_CANCELED + // and said the flag had been dropped, for a screen that was never asked to report one. + // Espresso's back goes through onBackPressed and therefore through the real exit. + androidx.test.espresso.Espresso.pressBackUnconditionally(); // ActivityScenario.getResult() hands back Instrumentation.ActivityResult, the same type // the stub above is built from. @@ -231,7 +234,7 @@ public void backingOutOfItDoesNotClaimAnImport() { onView(withId(R.id.settings_fetch_from_account)).perform(click()); Eventually.check(() -> intended(hasComponent(FetchFromICloudActivity.class.getName()))); - this.scenario.onActivity(Activity::finish); + androidx.test.espresso.Espresso.pressBackUnconditionally(); final android.content.Intent data = this.scenario.getResult().getResultData(); if (data != null) {