Draw the device serial per install, in both the app and the exporter

AGENTS.md

@@ -17,7 +17,7 @@ What that means in practice:
   does not belong. Rule 10 has the test.
 - **Do not carry this register outside the repository.** It is written for agents who need to be
   argued out of breaking something. A pull request review, an issue reply, or anything else a
-  person reads is a different job. See rule 16.
+  person reads is a different job. See rule 17.
 
 ## What this project is
 
@@ -239,9 +239,9 @@ Four things follow:
 
   **Today that is always `LEGACY_MAC`.** `Hardware.DEFAULT` is it, a fresh install gets it, and
   `IPHONE` is built and deliberately not chosen — see `generate()`. So every entry this project
-  registers currently appears as a `MacBookPro` on macOS 13.1, and the serial `0PENTAGVIEWR` is
-  the only thing distinguishing it from real hardware. The exporter is separately a MacBook Pro
-  on macOS 13.4.1, FindMy.py's own default, with serial `0PENTAGXPORT`.
+  registers currently appears as a `MacBookPro` on macOS 13.1, and the serial is the only thing
+  distinguishing it from real hardware. The exporter is separately a MacBook Pro on macOS 13.4.1,
+  FindMy.py's own default.
 
   Changing which profile a fresh install gets is not a cosmetic edit: it registers a *second*
   device rather than renaming the first, for anybody who reinstalls. That is why `IPHONE` exists
@@ -250,9 +250,24 @@ Four things follow:
   describe one real release ([findmy-export §2.2](./docs/findmy-export/01-authentication.md)).
   Claiming a Mac in one string and an iPhone in another is a contradiction Apple's own clients
   never produce.
-- **The serial is a label, and the only field here the user actually sees.** `0PENTAGVIEWR` is
-  confirmed accepted and displayed. Without one, Apple omits the row entirely, leaving an entry
-  with nothing to tell it apart from real hardware.
+- **The serial is a label, and the only field here the user actually sees.** Uppercase
+  alphanumeric, confirmed accepted and displayed. Without one, Apple omits the row entirely,
+  leaving an entry with nothing to tell it apart from real hardware.
+- **It is drawn per install, and the prefix is what carries the recognition.** The app sends
+  `0PENTAGV` plus four characters, the exporter `0PENTAGX` plus four, from an alphabet that
+  leaves out the pairs a person comparing two screens would confuse. **Do not put it back to a
+  constant.** It was one, and that meant a single serial arriving at Apple from thousands of
+  installs, against thousands of different machine identities and Apple IDs, from every continent
+  at once — a shape no real hardware produces, and the leading suspect for the Grand Slam 503s in
+  [#168](https://github.com/parawanderer/OpenTagViewer/issues/168),
+  [#176](https://github.com/parawanderer/OpenTagViewer/issues/176) and
+  [#181](https://github.com/parawanderer/OpenTagViewer/issues/181), one of whom cleared their
+  device identity to no effect — which regenerates the ids and not the serial.
+- **An install that already has one keeps it**, including the installs that predate this and have
+  no stored serial at all: those keep `0PENTAGVIEWR` / `0PENTAGXPORT`. Neither of those can be
+  drawn — both contain a letter the alphabet excludes — so a serial with an `I` or an `O` in it
+  is, by construction, an install from before the change. That is worth preserving when reading a
+  report, and `DrawingTheSerialTest` pins it.
 - **Sending the same value is not the same as sending the same bytes.** Two of these fields are
   transformed on the way out, and only by one side. FindMy.py sends `X-Apple-I-MD-LU` as
   `base64(uid)` and uppercases `X-Mme-Device-Id`; the Java ADI path sends what it is given. So
@@ -263,8 +278,12 @@ Four things follow:
   before believing two paths agree.
 
 **Changing it later adds an entry rather than renaming one**, and may require signing in again,
-so it is not a thing to adjust casually once shipped. Document what the app registers as, so a
-user reading their device list can recognise it — see the wiki.
+so it is not a thing to adjust casually once shipped — which is the whole reason the serial is
+drawn once, on the first run that needs an identity, and then persisted beside the rest of it
+(`LocalAnisette.KEY_SERIAL`, and `device-identity.json` for the exporter). A serial redrawn per
+sign-in would add a device-list entry every time, which is a worse bug than the constant was.
+Document what the app registers as, so a user reading their device list can recognise it — see
+the wiki, which names the prefix rather than a full serial for this reason.
 
 ### 12. A UI change gets a test that inflates it, and one that drives it
 
@@ -431,7 +450,34 @@ Every one of these is tested twice: `WhichFailuresNeedAFreshSignInTest` on the J
 decision, `EveryPathAsksForAFreshSignInTest` on a device for each caller honouring it. A shared
 predicate does not stop a fourth screen being written that never asks.
 
-### 16. Do not carry this file's voice into anything a person reads
+### 16. A pull request based on anything but `main` runs almost no CI
+
+Every workflow that matters here is gated `pull_request: branches: [ "main" ]` —
+`build-debug.yml`, `static-checks.yml`, `macos-scripts-python.yml`, and with them the APK build,
+the Chaquopy bridge tests, the JVM suite and the whole emulator suite. A PR opened against another
+branch, to stack a change on one still in review, matches none of those filters.
+
+**It does not report as skipped. It reports as green**, because the one workflow with no branch
+filter (`exporter-build-check.yml`) runs, passes, and is the only tick on the page. `gh pr checks`
+prints a single passing line and looks exactly like a small change with a small amount of CI.
+
+This has already happened: a change touching twelve Java files, four Python modules and six test
+classes sat on a PR based on another branch, with one green Windows-binary check and not one line
+of Java compiled anywhere.
+
+**So check what actually ran before believing a PR is green**, and count the checks rather than
+reading the colour:
+
+```bash
+gh pr checks <pr>          # one line is not a passing build, it is an empty one
+gh pr view <pr> --json baseRefName
+```
+
+Stacking is still fine — base it on `main` anyway. The diff carries the other branch's commits
+until that merges, which is cosmetic and collapses on its own; a PR whose base is not `main` buys
+a tidier diff by not being tested.
+
+### 17. Do not carry this file's voice into anything a person reads
 
 **@parawanderer has not read this file**, nor most of `docs/`, most docstrings, or most commit
 messages. Agents wrote them. So an agent reading them cannot tell the maintainer's house style from

app/build.gradle.kts

@@ -407,7 +407,7 @@ chaquopy {
             // wheel for desktop platforms and a pure-Python `py3-none-any` one as well.
             // There is no Android wheel, so pip falls back to the pure-Python build - which
             // is correct but markedly slower. The messages here are small enough not to care.
-            install("git+https://github.com/parawanderer/FindMy.py@ddc7f2342fc9f32ebe315b85c22a4554ce419f6d")
+            install("git+https://github.com/parawanderer/FindMy.py@3c2b4926252193e9cd265b39fa52252adcbaad4e")
 
             install("NSKeyedUnArchiver==1.5")
 

app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java

@@ -13,6 +13,7 @@
 import static androidx.test.espresso.matcher.ViewMatchers.withText;
 import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -309,6 +310,38 @@ public void anunrecognisedFailureStillShowsItsDetail() {
                 .check(matches(withText(containsString("Bad password")))));
     }
 
+    /**
+     * Apple declining is told apart from a wrong password and from a dead network.
+     *
+     * <p><b>Issue #176.</b> The screen echoed the failure verbatim, so somebody entering correct
+     * credentials during a Grand Slam outage was shown a sentence about HTTP 503 and an Apple
+     * internal service name. It reads as a bug in this app, and was filed as one.
+     *
+     * <p>Asserted on all three halves, because each is a different wrong turn: that it does not
+     * blame the password, that it does not send them to check a working connection, and that the
+     * raw protocol text is gone from the screen.
+     */
+    @Test
+    public void appleDecliningIsNotBlamedOnThePasswordOrTheNetwork() {
+        this.apple = FakeAppleAuthService.appleIsDeclining();
+        AppDependencies.replaceAuthService(this.apple);
+
+        launch();
+        signIn();
+
+        final android.content.Context context = getInstrumentation().getTargetContext();
+
+        Eventually.check(() -> onView(withId(R.id.login_error_message_text)).check(matches(
+                withText(context.getString(R.string.login_failed_apple_declined)))));
+
+        onView(withId(R.id.login_error_message_text)).check(matches(
+                not(withText(context.getString(R.string.login_failed_network)))));
+        onView(withId(R.id.login_error_message_text)).check(matches(
+                not(withText(containsString("503")))));
+        onView(withId(R.id.login_error_message_text)).check(matches(
+                not(withText(containsString("Grand Slam")))));
+    }
+
     /** A rejected code says so, and gives the boxes back rather than stranding them. */
     @Test
     public void aWrongCodeIsReportedAndTheBoxesComeBack() {

app/src/androidTest/java/dev/wander/android/opentagviewer/FetchFromICloudFlowTest.java

@@ -87,6 +87,44 @@ public void forgetAnyStoredMembership() {
                 .forget().blockingAwait();
     }
 
+    /**
+     * A serial only the generator could have produced, stored before the screen is opened.
+     *
+     * <p><b>Not {@link AdiDeviceIdentity#LEGACY_SERIAL}</b>, because that is what the screen
+     * falls back to when nothing is stored - so a test asserting the fallback would pass against
+     * a screen that never read this install's identity at all, which is the regression that
+     * matters: the app registers under one serial and the screen names another, and the row the
+     * user finds looks like somebody else's device.
+     */
+    private static final String THE_STORED_SERIAL = "0PENTAGVK7QX";
+
+    @org.junit.Before
+    public void giveThisInstallASerialToShow() {
+        final android.content.Context context =
+                InstrumentationRegistry.getInstrumentation().getTargetContext();
+
+        context.getSharedPreferences(
+                        dev.wander.android.opentagviewer.anisette.LocalAnisette.PREFERENCES,
+                        android.content.Context.MODE_PRIVATE)
+                .edit()
+                .putString(
+                        dev.wander.android.opentagviewer.anisette.LocalAnisette.KEY_SERIAL,
+                        THE_STORED_SERIAL)
+                .commit();
+    }
+
+    /** The identity is shared with the rest of the suite; one left behind is adopted as real. */
+    @After
+    public void takeTheSerialBackOut() {
+        InstrumentationRegistry.getInstrumentation().getTargetContext()
+                .getSharedPreferences(
+                        dev.wander.android.opentagviewer.anisette.LocalAnisette.PREFERENCES,
+                        android.content.Context.MODE_PRIVATE)
+                .edit()
+                .remove(dev.wander.android.opentagviewer.anisette.LocalAnisette.KEY_SERIAL)
+                .commit();
+    }
+
     private void open(final FakeICloudService fake) {
         this.icloud = fake;
         AppDependencies.replaceICloud(() -> fake);
@@ -209,13 +247,15 @@ public void theresultsSayWhatIsNowOnTheAppleAccount() {
                 .check(matches(withText(containsString(hardware.marketingName()))));
         onView(withId(R.id.icloud_registered_device_model))
                 .check(matches(withText(containsString(hardware.osVersion()))));
+        // The serial this install stored, not a constant: it is drawn per install now, so a
+        // screen showing a literal would send the user looking for a row that is not theirs.
         onView(withId(R.id.icloud_registered_device_serial))
-                .check(matches(withText(containsString(AdiDeviceIdentity.APP_SERIAL))));
+                .check(matches(withText(containsString(THE_STORED_SERIAL))));
         TestPace.afterAStep();
 
         onView(withId(R.id.icloud_registered_body)).perform(scrollTo());
         onView(withId(R.id.icloud_registered_body))
-                .check(matches(withText(containsString(AdiDeviceIdentity.APP_SERIAL))));
+                .check(matches(withText(containsString(THE_STORED_SERIAL))));
 
         // The serial reaches the sentence as well as the tile. The resource holds a ^1 slot, and
         // a template that lost it expands to a sentence about "the serial" that never says which.
@@ -353,6 +393,35 @@ public void aserviceHavingABadDayOffersARetryInstead() {
         TestPace.afterAStep();
     }
 
+    /**
+     * Apple declining reaches the retry screen, saying which of the two it is.
+     *
+     * <p><b>Issue #176.</b> Before this it fell through to the default branch, which puts the
+     * failure's own detail on screen - an HTTP status and Apple's internal service name. Correct,
+     * unreadable, and indistinguishable from a bug in this app.
+     *
+     * <p>Asserted on the raw text being absent as well as the sentence being present, because a
+     * branch that showed both would pass a check for only the second.
+     */
+    @Test
+    public void appleDecliningSaysSoRatherThanShowingTheStatusCode() {
+        this.open(FakeICloudService.whereAppleIsDeclining());
+
+        final android.content.Context context =
+                androidx.test.platform.app.InstrumentationRegistry
+                        .getInstrumentation().getTargetContext();
+
+        Eventually.check(() -> onView(withId(R.id.icloud_retry_container))
+                .check(matches(isDisplayed())));
+        Eventually.check(() -> onView(withId(R.id.icloud_retry_body)).check(matches(
+                withText(context.getString(R.string.icloud_apple_declined_body)))));
+
+        onView(withId(R.id.icloud_retry_body)).check(matches(
+                not(withText(org.hamcrest.Matchers.containsString("503")))));
+        onView(withId(R.id.icloud_no_tags_container)).check(matches(not(isDisplayed())));
+        TestPace.afterAStep();
+    }
+
     /** Retrying starts a fresh session rather than reusing the one that failed. */
     @Test
     public void retryingAsksAgain() {

app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/FakeAnisetteSource.java

@@ -131,6 +131,20 @@ public String deviceIdsJson() {
         return "{\"uid\":\"" + UID + "\",\"devid\":\"" + DEVID + "\"}";
     }
 
+    /**
+     * A drawn serial, not {@link AdiDeviceIdentity#LEGACY_SERIAL}.
+     *
+     * <p>Deliberately a value only the generator could produce, so a test asserting that this
+     * reached Apple cannot pass against code that fell back to the constant - which is the
+     * regression that matters here, and the one a fake returning the old literal would hide.
+     */
+    public static final String SERIAL = "0PENTAGVK7QX";
+
+    @Override
+    public String serial() {
+        return SERIAL;
+    }
+
     @Override
     public String describe() {
         return this.ready ? "fake, ready" : "fake, unavailable: " + this.unavailableReason;

app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java

@@ -5,6 +5,7 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import android.content.Context;
@@ -280,4 +281,85 @@ public void ahalfWrittenIdentityIsTreatedAsAbsent() {
         assertNotEquals(OLD_DEVICE_ID,
                 this.preferences.getString(LocalAnisette.KEY_DEVICE_ID, null));
     }
+
+    /**
+     * An install from before serials were drawn keeps the one it has been presenting.
+     *
+     * <p>Same shape as the hardware profile, and the same reason: three keys and no serial can
+     * only be an install that has been telling Apple {@code 0PENTAGVIEWR} for its whole life.
+     * Drawing it a new one now registers a <i>second</i> device beside the row the user already
+     * recognises, and may cost them a sign-in - to change a label.
+     */
+    @Test
+    public void anIdentityWrittenBeforeSerialsWereDrawnStillPresentsTheOldOne() {
+        writeTheOldShape();
+
+        assertEquals(AdiDeviceIdentity.LEGACY_SERIAL, subject().serial());
+    }
+
+    /** And it is not back-filled, so it stays distinguishable from a serial that was drawn. */
+    @Test
+    public void thelegacySerialIsNotWrittenBackOverTheOldShape() {
+        writeTheOldShape();
+
+        subject().serial();
+
+        assertNull("writing it back would make an old install look like one that drew it",
+                this.preferences.getString(LocalAnisette.KEY_SERIAL, null));
+    }
+
+    /**
+     * A fresh install draws one, and <b>keeps it</b>.
+     *
+     * <p>The keeping is the part that matters. A serial redrawn per sign-in would add a
+     * device-list entry every time, which is the failure the constant did not have and this
+     * change could easily introduce.
+     */
+    @Test
+    public void afreshInstallDrawsASerialAndThenKeepsIt() {
+        final String drawn = subject().serial();
+
+        assertTrue("a drawn serial has to be recognisable as this app",
+                drawn.startsWith(AdiDeviceIdentity.SERIAL_PREFIX));
+        assertNotEquals("a fresh install is not an install from before this change",
+                AdiDeviceIdentity.LEGACY_SERIAL, drawn);
+        assertEquals("it was stored, so a restart presents the same device",
+                drawn, this.preferences.getString(LocalAnisette.KEY_SERIAL, null));
+        assertEquals("asked twice, answered twice the same", drawn, subject().serial());
+    }
+
+    /** A stored serial is presented unchanged, whatever it is. */
+    @Test
+    public void astoredSerialIsWhatGoesToApple() {
+        writeTheOldShape();
+        assertTrue(this.preferences.edit()
+                .putString(LocalAnisette.KEY_SERIAL, "0PENTAGVK7QX").commit());
+
+        assertEquals("0PENTAGVK7QX", subject().serial());
+    }
+
+    /**
+     * The screen asks read-only, and a screen must not be what decides an install's identity.
+     *
+     * <p>Same contract as {@link LocalAnisette#profileToShow}: opening a page on a device that
+     * has never signed in would otherwise mint and store an identity, fixing what this install
+     * is by having looked at it.
+     */
+    @Test
+    public void showingTheSerialDoesNotMintOne() {
+        assertEquals(AdiDeviceIdentity.LEGACY_SERIAL, LocalAnisette.serialToShow(this.context));
+
+        assertNull("reading it for a label must not write one",
+                this.preferences.getString(LocalAnisette.KEY_SERIAL, null));
+        assertNull(this.preferences.getString(LocalAnisette.KEY_DEVICE_ID, null));
+    }
+
+    /** And once there is one, that is what the screen shows. */
+    @Test
+    public void showingTheSerialShowsTheOneThisInstallWillSend() {
+        final String drawn = subject().serial();
+
+        assertEquals("the screen named a serial Apple never saw for this user",
+                drawn, LocalAnisette.serialToShow(this.context));
+    }
 }