Blocking someone showed an error, and could be undone by their next message

Found by finally running the banner I shipped in 0.3 instead of trusting that
it compiled.

`blocked()` read `user_id` from rows the relay emits as `{id, name, slug}` —
the buddy-list shape, not the `user_id` shape the §8 routes use. It threw
`No value for user_id`. Tapping Block worked on the server and then showed the
user an error, which on a safety action is the worst outcome available: it
succeeded and the screen said otherwise.

The reason it survived is the reason it was dangerous. The block list is empty
until somebody blocks their first person, so the map never ran and every test
passed. The first block anybody performed broke it.

🔴 And it did not stop at a message. `adoptStrangers` read the block list with
`.getOrDefault(emptyList())`, so the throw became "nobody is blocked" — and the
next message from someone you had just blocked auto-added them back to your
contacts, defeating the exact property the code comment above it claims to
hold. That now fails closed: if the block list cannot be established, nothing
is adopted and the next poll tries again. A stranger appearing a few seconds
late is cosmetic; re-adding somebody who blocked their way out of a
conversation is not.

The new test uses a NON-EMPTY list, because that is the case that was missing —
an empty-list assertion looks like coverage and exercises no parsing. Confirmed
by reintroducing the bug: the populated case fails, the empty case still
passes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

app/build.gradle.kts

@@ -33,8 +33,8 @@ android {
         // ⚠️ Android also refuses to install an APK whose `versionCode` is
         // lower than the installed one, so a code that goes backwards makes a
         // release uninstallable for everybody already on the higher number.
-        versionCode = 3
-        versionName = "0.3"
+        versionCode = 4
+        versionName = "0.4"
         testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"
     }
 

app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt

@@ -1279,7 +1279,14 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) {
         adopting += strangers
         viewModelScope.launch(Dispatchers.IO) {
             var added = false
-            val blocked = runCatching { current.api.blocked() }.getOrDefault(emptyList())
+            // 🔴 **Fail closed.** This used to default to "nobody is blocked"
+            // when the call failed, which is the wrong direction for a safety
+            // check: a parse bug in the block list silently became "adopt the
+            // person you blocked". If we cannot establish who is blocked, we
+            // adopt nobody and try again on the next poll — a stranger showing
+            // up a few seconds late is a cosmetic problem; re-adding somebody
+            // who blocked their way out of a conversation is not.
+            val blocked = runCatching { current.api.blocked() }.getOrNull() ?: return@launch
             for (id in strangers) {
                 runCatching {
                     if (id in blocked) return@runCatching

client/src/main/kotlin/io/flamenet/client/MessengerApi.kt

@@ -184,9 +184,23 @@ class MessengerApi(
 
     fun markAllNotificationsRead() = post("notifications/read_all", JSONObject()).let { }
 
+    /**
+     * The ids this account has blocked.
+     *
+     * 🔴 **`id`, not `user_id`.** This route answers `{id, name, slug}` — the
+     * same shape as the buddy list and NOT the `user_id` shape the §8 routes
+     * use. Reading `user_id` here threw `No value for user_id`, and the reason
+     * it survived so long is the reason it was dangerous: the list is empty
+     * until somebody blocks their first person, so the map never ran and every
+     * test passed. The first block anybody performed broke it.
+     *
+     * ⚠️ It threw where a caller was defaulting to "nobody is blocked", which
+     * turned a field-name typo into a blocked person being auto-added back to
+     * your contacts. See `adoptStrangers`.
+     */
     fun blocked(): List<Long> {
         val list = get("blocks").optJSONArray("blocked") ?: JSONArray()
-        return (0 until list.length()).map { list.getJSONObject(it).getLong("user_id") }
+        return (0 until list.length()).map { list.getJSONObject(it).getLong("id") }
     }
 
     fun setBlocked(userId: Long, blocked: Boolean) {

client/src/test/kotlin/io/flamenet/client/BlockListShapeTest.kt

@@ -0,0 +1,68 @@
+package io.flamenet.client
+
+import okhttp3.mockwebserver.MockResponse
+import okhttp3.mockwebserver.MockWebServer
+import kotlin.test.AfterTest
+import kotlin.test.BeforeTest
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+/**
+ * The block list's wire shape, parsed from a NON-EMPTY list.
+ *
+ * 🔴 **This is the test that was missing, and its absence had teeth.** The
+ * client read `user_id` from rows the relay emits as `{id, name, slug}` — the
+ * buddy-list shape, not the `user_id` shape the §8 routes use. It threw
+ * `No value for user_id`, and it survived because the only thing ever asserted
+ * was an EMPTY block list: with no rows, the map never runs and the parser is
+ * never exercised. Everything passed until the first person blocked somebody.
+ *
+ * ⚠️ So the case that matters is a list with something in it. An empty-list
+ * assertion here would be worse than none, because it looks like coverage.
+ */
+class BlockListShapeTest {
+
+    private lateinit var server: MockWebServer
+
+    @BeforeTest fun start() { server = MockWebServer().also { it.start() } }
+    @AfterTest fun stop() { server.shutdown() }
+
+    private fun api() = MessengerApi(
+        server.url("/").toString().trimEnd('/'), "someone", "app-password")
+
+    private fun serve(body: String) {
+        server.enqueue(MockResponse().setResponseCode(200).setBody(body))
+    }
+
+    @Test
+    fun `a populated block list parses, using id rather than user_id`() {
+        // Exactly what the relay emits — see BlockListResponse in Wire.swift
+        // and the conformance assertion "the block list carries id, name and
+        // slug".
+        serve("""{"blocked":[
+            {"id": 42, "name": "Spam Lord", "slug": "spamlord"},
+            {"id": 77, "name": "Someone Else", "slug": "someone-else"}
+        ]}""")
+        assertEquals(listOf(42L, 77L), api().blocked())
+    }
+
+    @Test
+    fun `an empty block list is empty rather than an error`() {
+        serve("""{"blocked":[]}""")
+        assertTrue(api().blocked().isEmpty())
+    }
+
+    /**
+     * ⚠️ A row shaped like the §8 routes must NOT quietly parse. If the relay
+     * ever changed shape, silently reading a different field would hand back
+     * an under-populated block list — and an under-populated block list is a
+     * blocked person becoming visible again.
+     */
+    @Test
+    fun `a row without id is a failure, not a silent gap in the list`() {
+        serve("""{"blocked":[{"user_id": 42, "name": "Spam Lord"}]}""")
+        val outcome = runCatching { api().blocked() }
+        assertTrue(outcome.isFailure, "a row with no `id` must not parse to something")
+    }
+}