Diagnostics: Refuse to deliver a report that carries no settings

A discovery report is only actionable if it names at least one candidate
key. Two shapes reached delivery without one, and both cost a maintainer
a round-trip that ends in "there is nothing here".

An empty matrix was deliverable on purpose: "this ROM keeps the mode
elsewhere" is a real result. In practice it is indistinguishable from the
two capture mishaps that produce the same output, so the negative result
was never trustworthy enough to pay for the reports it let through.
REVIEW now has no forward path at all when nothing differed, and the card
says so instead of offering "Continue anyway".

The second shape is a matrix whose rows were all withheld. A new device's
key is never on the auto-disclose allowlist by construction, so the
reports worth the most are exactly the ones that arrive empty when the
contributor skips the reveal step. Delivery now needs one included row,
with a card naming the step rather than leaving Next disabled and
unexplained. Withholding individual rows is untouched.

Both gates are one predicate, ContributionUiState.deliverable.

Author: d4rken

app/src/main/java/eu/darken/amply/diagnostics/ui/ContributionWizardScreen.kt

@@ -102,15 +102,12 @@ fun ContributionWizardScreen(
                     // Not while a capture is in flight — Review must reflect a settled session. Below two modes there
                     // is nothing to diff, so Review could only ever be empty.
                     WizardStep.CAPTURE -> state.modes.size >= ContributionWizardViewModel.MIN_MODES && !state.busy
+                    // No settings, no delivery. Both dead ends (nothing differed, or everything withheld) are
+                    // explained by a card in the step itself, so a disabled Next is never unexplained.
+                    WizardStep.REVIEW -> state.deliverable
                     else -> true
                 },
-                // An empty matrix is still deliverable — "the ROM stores this elsewhere" is a real finding — but the
-                // label has to stop reading like a normal happy-path Next.
-                nextLabel = if (state.step == WizardStep.REVIEW && state.review.isEmpty()) {
-                    R.string.contribution_next_empty
-                } else {
-                    R.string.contribution_next
-                },
+                nextLabel = R.string.contribution_next,
                 onBack = onBack,
                 onNext = onNext,
             )
@@ -423,16 +420,19 @@ private fun LazyListScope.reviewStep(
         item { EmptyReviewCard(onRestart) }
     } else {
         item { BodyText(stringResource(R.string.contribution_review_body)) }
+        if (state.nothingIncluded) {
+            item { NothingIncludedCard() }
+        }
         items(state.review, key = { it.id.display }) { row ->
             ReviewRowCard(row, onRevealRow, onToggleInclude)
         }
     }
 }
 
 /**
- * Shown when the captured modes produced no differences at all. Most reports that land here are a capture mishap rather
- * than a finding, so the card names the likely causes — but it deliberately does not block delivery: "this ROM keeps the
- * mode somewhere else" is exactly the kind of result that should still reach a maintainer.
+ * Shown when the captured modes produced no differences at all. There is nothing to send from here: the report would
+ * name no candidate key, and a maintainer cannot tell a capture mishap from a ROM that hides the setting. So the card
+ * names the likely causes and the only two ways forward — capture again, or accept the device isn't mappable this way.
  */
 @Composable
 private fun EmptyReviewCard(onRestart: () -> Unit) {
@@ -456,6 +456,25 @@ private fun EmptyReviewCard(onRestart: () -> Unit) {
     }
 }
 
+/**
+ * Rows were found but none are included, so the report would carry no candidate key. A new device's key is never on the
+ * auto-disclose allowlist by definition, so this is the normal state of exactly the reports worth the most — the card
+ * has to explain the reveal step rather than leave a disabled Next unexplained.
+ */
+@Composable
+private fun NothingIncludedCard() {
+    AmplyCard(
+        tone = AmplyCardTone.TertiaryContainer,
+        verticalArrangement = Arrangement.spacedBy(AmplyCardDefaults.ItemSpacing),
+    ) {
+        Text(
+            stringResource(R.string.contribution_review_nothing_included_title),
+            style = MaterialTheme.typography.titleMedium,
+        )
+        Text(stringResource(R.string.contribution_review_nothing_included_body))
+    }
+}
+
 @Composable
 private fun ReviewRowCard(
     row: ReviewRowUi,

app/src/main/java/eu/darken/amply/diagnostics/ui/ContributionWizardViewModel.kt

@@ -74,6 +74,17 @@ data class ContributionUiState(
      * and the ROM version, and those two often identify the ROM family on their own.
      */
     val detailsComplete: Boolean get() = featureName.isNotBlank() && romVersion.isNotBlank()
+
+    /**
+     * A report is only worth delivering if it carries at least one setting. This covers both dead ends with one
+     * predicate: an empty matrix (nothing differed across the captured modes) leaves [review] empty, and a matrix
+     * whose every row was withheld leaves nothing included. Either way the report would name no candidate key, which
+     * is the only thing a maintainer can act on.
+     */
+    val deliverable: Boolean get() = review.any { it.included }
+
+    /** Rows were found, but none are included yet — distinct from [review] being empty, and fixable by the user. */
+    val nothingIncluded: Boolean get() = review.isNotEmpty() && !deliverable
 }
 
 /**
@@ -218,7 +229,9 @@ class ContributionWizardViewModel @Inject constructor(
             WizardStep.CAPTURE -> if (rawSession.observations.size >= MIN_MODES && captureJob?.isActive != true) {
                 buildReview()
             }
-            WizardStep.REVIEW -> buildDelivery()
+            // A report with no settings is not deliverable at all: there is no "send it anyway" path, because an
+            // empty report costs a maintainer a round-trip and tells them nothing the device list doesn't already.
+            WizardStep.REVIEW -> if (mutableState.value.deliverable) buildDelivery()
             WizardStep.DELIVER -> Unit
         }
     }

app/src/main/res/values/strings.xml

@@ -335,7 +335,9 @@
     <string name="contribution_review_empty_title">No differences found</string>
     <string name="contribution_review_empty_body">Nothing changed in the secure, global, or system settings between the modes you captured. That usually means one of:</string>
     <string name="contribution_review_empty_causes">• The mode wasn\'t actually switched in the system settings between captures.\n• It was switched, but captured before the system applied it. Wait until the manufacturer\'s screen shows the new mode, then capture.\n• This ROM keeps the mode somewhere Amply can\'t read.</string>
-    <string name="contribution_review_empty_hint">Only the last one is worth sending. If you\'re not sure, start over and capture each mode again.</string>
+    <string name="contribution_review_empty_hint">There\'s nothing to send from this run, so this is as far as it goes. Start over and capture each mode again if you think one of the first two applies. If your phone has no battery-protection setting to switch, or it keeps it somewhere Amply can\'t read, then this device can\'t be mapped this way.</string>
+    <string name="contribution_review_nothing_included_title">Nothing included yet</string>
+    <string name="contribution_review_nothing_included_body">Settings did change, but none are in the report yet. Support for a new device is usually hiding behind one of the rows below, so reveal them, check that nothing personal is in there, and include at least one. A report with no settings can\'t be used.</string>
 
     <!-- Step: deliver -->
     <string name="contribution_deliver_title">Send it</string>
@@ -348,7 +350,6 @@
 
     <!-- Navigation -->
     <string name="contribution_next">Next</string>
-    <string name="contribution_next_empty">Continue anyway</string>
     <string name="contribution_back">Back</string>
 
     <!-- Dashboard (static UI) -->

app/src/test/java/eu/darken/amply/diagnostics/ui/ContributionWizardEmptyReviewTest.kt

@@ -81,22 +81,39 @@ class ContributionWizardEmptyReviewTest {
         values = listOf("0", "1"),
     )
 
+    /** An unknown key: redacted, not revealed, not included — the default state of a new device's real mapping row. */
+    private val withheldRow = ReviewRowUi(
+        id = SettingId(SettingNamespace.SECURE, "vendor_unknown_charge_key"),
+        disclosure = Disclosure.REDACTED,
+        revealed = false,
+        included = false,
+        values = null,
+    )
+
     @Test
-    fun `an empty review explains itself instead of reading like a normal step`() {
+    fun `an empty review explains itself and cannot be delivered`() {
         render(reviewState(emptyList()))
 
         compose.onNodeWithText(string(R.string.contribution_review_empty_title)).assertExists()
-        compose.onNodeWithText(string(R.string.contribution_next_empty)).assertExists()
-        // The happy-path label must be gone — that is the whole point of the relabel.
-        compose.onNodeWithText(string(R.string.contribution_next)).assertDoesNotExist()
+        // No "send it anyway" path: a report with no settings never reaches an issue or the support inbox.
+        compose.onNodeWithText(string(R.string.contribution_next)).assertIsNotEnabled()
     }
 
     @Test
-    fun `a populated review keeps the normal next label`() {
+    fun `a review with rows but nothing included cannot be delivered`() {
+        render(reviewState(listOf(withheldRow)))
+
+        compose.onNodeWithText(string(R.string.contribution_review_nothing_included_title)).assertExists()
+        compose.onNodeWithText(string(R.string.contribution_next)).assertIsNotEnabled()
+    }
+
+    @Test
+    fun `a populated review can be delivered`() {
         render(reviewState(listOf(populatedRow)))
 
-        compose.onNodeWithText(string(R.string.contribution_next)).assertExists()
+        compose.onNodeWithText(string(R.string.contribution_next)).assertIsEnabled()
         compose.onNodeWithText(string(R.string.contribution_review_empty_title)).assertDoesNotExist()
+        compose.onNodeWithText(string(R.string.contribution_review_nothing_included_title)).assertDoesNotExist()
     }
 
     // Tall screen: at Robolectric's 470px default the card's action lands behind the bottom bar and the click would

app/src/test/java/eu/darken/amply/diagnostics/ui/ContributionWizardViewModelTest.kt

@@ -183,6 +183,43 @@ class ContributionWizardViewModelTest {
         vm.state.value.review.shouldBeEmpty()
     }
 
+    @Test
+    fun `an empty matrix cannot be delivered`() = runTest(dispatcher.scheduler) {
+        val same = mapOf(global("protect_battery") to "0")
+        val vm = reachedReviewWith(a = same, b = same)
+        vm.state.value.deliverable shouldBe false
+
+        vm.goNext()
+        advanceUntilIdle()
+
+        vm.state.value.step shouldBe WizardStep.REVIEW
+        vm.state.value.reportText shouldBe null
+        vm.state.value.issueUrl shouldBe null
+    }
+
+    @Test
+    fun `a matrix whose rows are all withheld cannot be delivered until one is included`() =
+        runTest(dispatcher.scheduler) {
+            // An unknown key is redacted by default, which is exactly what a new device's real mapping row looks like.
+            val id = secure("vendor_unknown_charge_key")
+            val vm = reachedReviewWith(a = mapOf(id to "0"), b = mapOf(id to "1"))
+            vm.state.value.review.single().included shouldBe false
+            vm.state.value.nothingIncluded shouldBe true
+
+            vm.goNext()
+            advanceUntilIdle()
+            vm.state.value.step shouldBe WizardStep.REVIEW
+
+            vm.revealRow(id)
+            vm.toggleInclude(id)
+            vm.state.value.deliverable shouldBe true
+
+            vm.goNext()
+            advanceUntilIdle()
+            vm.state.value.step shouldBe WizardStep.DELIVER
+            vm.state.value.reportText.shouldNotBeNull() shouldContain "vendor_unknown_charge_key"
+        }
+
     @Test
     fun `restarting from an empty review clears the captures and returns to capture`() = runTest(dispatcher.scheduler) {
         val same = mapOf(global("protect_battery") to "0")

docs/support/kindmail-system-prompt.md

@@ -131,9 +131,12 @@ that their device will be supported.
 
 ### If `changed_rows=0`
 
-Not actionable by itself. The wizard already showed them a "No differences found" warning with the likely
-causes and they chose "Continue anyway", so **do not tell them they did it wrong or that they skipped a step.**
-They didn't.
+**Current builds cannot produce this.** Since the wizard change that followed the first of these emails, a
+capture that finds no differences cannot be delivered at all: no issue, no email, only "Start over". A
+`changed_rows=0` report arriving now means the user is on 0.2.1-beta0 or older, so ask them to update first.
+
+Not actionable by itself. Older builds warned about it and then offered a "Continue anyway" button, so **do not
+tell them they did it wrong or that they skipped a step.** They used a path the app handed them.
 
 There are three causes, and only the last one is a real finding:
 
@@ -158,6 +161,12 @@ Then check two things in the block and work them into the reply:
 
 - A report with fewer than two captured modes can't be produced by current versions; the wizard blocks it. If
   one turns up anyway, the user is on an old build, ask them to update.
+- Same for `(no settings approved for inclusion)`, which means settings changed but the contributor included
+  none of them. Current builds block delivery until at least one row is included, so this shape also only
+  arrives from 0.2.1-beta0 or older. Don't push the user to disclose anything; just ask them to update and run
+  it again, and the wizard will explain the reveal step in place.
+- Taken together: every discovery report from a current build carries at least one setting. If one doesn't, the
+  build is old, and that is the first thing to check.
 - Never tell a user their device will be supported on the strength of a report. The gate is physical
   verification, not a settings mapping.