Diagnostics: Stop reporting an empty capture as a withheld one (#31)

A contribution report with no rows printed "(no settings approved for
inclusion)" regardless of cause. In issue #23 the matrix was empty --
nothing differed across the captured modes -- but the wording reads as a
contributor redacting every row, so the only way to tell the two apart
was the absence of an optional withheld_rows line.

Report schema 2 states what was measured: scanned_namespaces,
captured_mode_count, and changed_rows. An empty matrix now says so in
its own words and names the providers that were actually compared;
all-withheld keeps the inclusion wording.

The wizard also let two paths reach that report unchallenged. A single
capture could advance to review, where a diff has nothing to diff
against and the matrix is empty by construction -- now blocked at two
observations in the ViewModel, with the UI mirroring the guard. A
genuinely empty two-mode result stays deliverable, because "this ROM
keeps the mode elsewhere" is a real finding, but the review step
explains the likely causes, offers a restart, and relabels the primary
action to "Continue anyway".

A backend only reports Failure when the call threw; empty or unparsable
output parses to an empty map instead, and three empty namespaces would
diff to "nothing changed" and look like that same real finding. A merged
snapshot with no keys at all is now a capture failure.

Author: d4rken

app/src/main/java/eu/darken/amply/diagnostics/core/ContributionModels.kt

@@ -53,6 +53,10 @@ data class ReviewedRow(
  * The complete, immutable, export-safe contribution report. The pure formatter accepts **only** this type and has no
  * access to raw snapshots, so privacy cannot regress through a later formatting change. Redacted rows are represented
  * solely by [withheldRowCount] — their namespace, key, and values are absent entirely.
+ *
+ * [changedRowCount] and [scannedNamespaces] describe the *measurement*, not its contents: without them a report with no
+ * rows is ambiguous between "nothing differed across the modes" and "the contributor withheld everything", and a reader
+ * cannot tell which providers were even looked at. Both are counts/namespace names only — never key or value bearing.
  */
 data class ReviewedContributionReport(
     val schema: Int,
@@ -70,6 +74,8 @@ data class ReviewedContributionReport(
     val modeLabels: List<String>,
     val rows: List<ReviewedRow>,
     val withheldRowCount: Int,
+    val changedRowCount: Int,
+    val scannedNamespaces: List<String>,
     val effects: List<ModeEffect>,
     val notes: String,
 )

app/src/main/java/eu/darken/amply/diagnostics/core/ContributionReport.kt

@@ -1,11 +1,14 @@
 package eu.darken.amply.diagnostics.core
 
 import eu.darken.amply.charging.core.DeviceInfo
+import eu.darken.amply.charging.core.access.STANDARD_SETTINGS_NAMESPACES
+import eu.darken.amply.charging.core.access.SettingNamespace
 import eu.darken.amply.main.core.DeviceSupportReporter
 import eu.darken.amply.main.core.sanitizeReportValue
 import java.net.URLEncoder
 
-internal const val CONTRIBUTION_SCHEMA = 1
+/** 2: added `scanned_namespaces`, `captured_mode_count`, `changed_rows`, and a distinct empty-matrix wording. */
+internal const val CONTRIBUTION_SCHEMA = 2
 
 /** Conservative cap for a GitHub issue prefill URL; browsers/GitHub start truncating well above this. */
 internal const val MAX_ISSUE_URL_BYTES = 6_000
@@ -46,6 +49,7 @@ internal fun buildReviewedReport(
     effects: List<ModeEffect>,
     notes: String,
     createdAtEpochMs: Long,
+    scannedNamespaces: List<SettingNamespace> = STANDARD_SETTINGS_NAMESPACES,
     schema: Int = CONTRIBUTION_SCHEMA,
 ): ReviewedContributionReport {
     val matrix = deriveMatrix(session.observations)
@@ -72,6 +76,8 @@ internal fun buildReviewedReport(
             )
         },
         withheldRowCount = matrix.size - exportRows.size,
+        changedRowCount = matrix.size,
+        scannedNamespaces = scannedNamespaces.map { it.commandName },
         effects = effects.map { ModeEffect(sanitizeReportValue(it.modeLabel), sanitizeReportValue(it.effect)) },
         notes = sanitizeReportValue(notes, MAX_NOTE),
     )
@@ -92,10 +98,23 @@ internal fun formatContributionReport(report: ReviewedContributionReport): Strin
     appendLine("rom_version=${report.romVersion.ifBlank { "unspecified" }}")
     appendLine("feature_name=${report.featureName.ifBlank { "unspecified" }}")
     appendLine("modes=${report.modeLabels.joinToString(" | ")}")
+    appendLine("captured_mode_count=${report.modeLabels.size}")
+    appendLine("scanned_namespaces=${report.scannedNamespaces.joinToString(",")}")
+    appendLine("changed_rows=${report.changedRowCount}")
     appendLine()
     appendLine("# changed settings (value per mode, in the order above)")
     if (report.rows.isEmpty()) {
-        appendLine("(no settings approved for inclusion)")
+        // Two very different outcomes used to share one line. An empty matrix is a *measurement* result (nothing moved
+        // in the scanned providers); an all-withheld matrix is a *contributor* choice. Reading the first as the second
+        // sends a maintainer chasing a privacy decision that never happened.
+        if (report.changedRowCount == 0) {
+            appendLine(
+                "(no settings changed across the captured modes — " +
+                    "nothing differed in ${report.scannedNamespaces.joinToString(", ")})",
+            )
+        } else {
+            appendLine("(no settings approved for inclusion)")
+        }
     } else {
         report.rows.forEach { row ->
             appendLine("${row.namespace}/${row.key} = ${row.valuesByMode.joinToString(" | ") { it ?: "<absent>" }}")

app/src/main/java/eu/darken/amply/diagnostics/core/ContributionRepository.kt

@@ -2,13 +2,15 @@ package eu.darken.amply.diagnostics.core
 
 import android.content.Context
 import dagger.hilt.android.qualifiers.ApplicationContext
+import eu.darken.amply.R
 import eu.darken.amply.charging.core.DeviceInfo
 import eu.darken.amply.charging.core.access.BackendStatus
 import eu.darken.amply.charging.core.access.NamespaceSnapshot
 import eu.darken.amply.charging.core.access.STANDARD_SETTINGS_NAMESPACES
 import eu.darken.amply.charging.core.access.SettingsSnapshotSource
 import eu.darken.amply.charging.core.adapter.AdapterRegistry
 import eu.darken.amply.common.ca.CaString
+import eu.darken.amply.common.ca.toCaString
 import javax.inject.Inject
 import javax.inject.Singleton
 
@@ -50,6 +52,10 @@ class DefaultContributionRepository @Inject constructor(
                 is NamespaceSnapshot.Failure -> return CaptureResult.Failure(result.reason)
             }
         }
+        // A backend only reports Failure when the call *threw*. A command that returns empty or unparsable output
+        // instead parses to an empty map, and three empty namespaces would look exactly like a valid "nothing changed"
+        // capture. No real Android device has zero settings across all three, so treat it as a failed read.
+        if (merged.isEmpty()) return CaptureResult.Failure(R.string.contribution_capture_empty_error.toCaString())
         return CaptureResult.Success(merged)
     }
 

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

@@ -1,5 +1,6 @@
 package eu.darken.amply.diagnostics.ui
 
+import androidx.annotation.StringRes
 import androidx.compose.foundation.layout.Arrangement
 import androidx.compose.foundation.layout.ExperimentalLayoutApi
 import androidx.compose.foundation.layout.FlowRow
@@ -39,6 +40,7 @@ import eu.darken.amply.R
 import eu.darken.amply.charging.core.access.BackendStatus
 import eu.darken.amply.common.compose.AmplyCard
 import eu.darken.amply.common.compose.AmplyCardDefaults
+import eu.darken.amply.common.compose.AmplyCardTone
 import eu.darken.amply.common.compose.AmplyCodeBlock
 import eu.darken.amply.common.compose.AmplyPreview
 import eu.darken.amply.common.compose.PreviewWrapper
@@ -94,10 +96,18 @@ fun ContributionWizardScreen(
                 showNext = state.step != WizardStep.DELIVER,
                 nextEnabled = when (state.step) {
                     WizardStep.INTRO -> state.shizukuReady
-                    // Not while a capture is in flight — Review must reflect a settled session.
-                    WizardStep.CAPTURE -> state.modes.isNotEmpty() && !state.busy
+                    // 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
                     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
+                },
                 onBack = onBack,
                 onNext = onNext,
             )
@@ -130,7 +140,7 @@ fun ContributionWizardScreen(
                     onUndoLast,
                     onRestart,
                 )
-                WizardStep.REVIEW -> reviewStep(state, onRevealRow, onToggleInclude)
+                WizardStep.REVIEW -> reviewStep(state, onRevealRow, onToggleInclude, onRestart)
                 WizardStep.DELIVER -> deliverStep(state, onOpenIssue, onCopyReport, onEmail)
             }
         }
@@ -142,6 +152,7 @@ private fun WizardBottomBar(
     showBack: Boolean,
     showNext: Boolean,
     nextEnabled: Boolean,
+    @StringRes nextLabel: Int,
     onBack: () -> Unit,
     onNext: () -> Unit,
 ) {
@@ -161,7 +172,7 @@ private fun WizardBottomBar(
             Spacer(Modifier.weight(1f))
             if (showNext) {
                 Button(onClick = onNext, enabled = nextEnabled) {
-                    Text(stringResource(R.string.contribution_next))
+                    Text(stringResource(nextLabel))
                 }
             }
         }
@@ -401,18 +412,46 @@ private fun LazyListScope.reviewStep(
     state: ContributionUiState,
     onRevealRow: (SettingId) -> Unit,
     onToggleInclude: (SettingId) -> Unit,
+    onRestart: () -> Unit,
 ) {
     item { SectionTitle(stringResource(R.string.contribution_review_title)) }
-    item { BodyText(stringResource(R.string.contribution_review_body)) }
     if (state.review.isEmpty()) {
-        item { BodyText(stringResource(R.string.contribution_review_empty)) }
+        item { EmptyReviewCard(onRestart) }
     } else {
+        item { BodyText(stringResource(R.string.contribution_review_body)) }
         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.
+ */
+@Composable
+private fun EmptyReviewCard(onRestart: () -> Unit) {
+    AmplyCard(
+        tone = AmplyCardTone.TertiaryContainer,
+        verticalArrangement = Arrangement.spacedBy(AmplyCardDefaults.ItemSpacing),
+    ) {
+        Text(
+            stringResource(R.string.contribution_review_empty_title),
+            style = MaterialTheme.typography.titleMedium,
+        )
+        Text(stringResource(R.string.contribution_review_empty_body))
+        Text(stringResource(R.string.contribution_review_empty_causes))
+        Text(
+            stringResource(R.string.contribution_review_empty_hint),
+            style = MaterialTheme.typography.bodySmall,
+        )
+        TextButton(onClick = onRestart) {
+            Text(stringResource(R.string.contribution_restart))
+        }
+    }
+}
+
 @Composable
 private fun ReviewRowCard(
     row: ReviewRowUi,
@@ -505,8 +544,8 @@ private fun LazyListScope.deliverStep(
 @AmplyPreview
 @Composable
 private fun ContributionWizardScreenPreview() = PreviewWrapper {
-    ContributionWizardScreen(
-        state = ContributionUiState(
+    PreviewScreen(
+        ContributionUiState(
             step = WizardStep.CAPTURE,
             shizuku = BackendStatus(available = true, granted = true, detail = "Shizuku connected".toCaString()),
             featureName = "Protect battery",
@@ -526,6 +565,31 @@ private fun ContributionWizardScreenPreview() = PreviewWrapper {
                 ),
             ),
         ),
+    )
+}
+
+@AmplyPreview
+@Composable
+private fun ContributionWizardEmptyReviewPreview() = PreviewWrapper {
+    PreviewScreen(
+        ContributionUiState(
+            step = WizardStep.REVIEW,
+            shizuku = BackendStatus(available = true, granted = true, detail = "Shizuku connected".toCaString()),
+            featureName = "Charging protection",
+            romVersion = "HyperOS 3",
+            modes = listOf(
+                ModeSummary(label = "Intelligent charging", effect = "no_change", changedFromPrevious = null),
+                ModeSummary(label = "Charge fully", effect = "no_change", changedFromPrevious = 0),
+            ),
+            review = emptyList(),
+        ),
+    )
+}
+
+@Composable
+private fun PreviewScreen(state: ContributionUiState) {
+    ContributionWizardScreen(
+        state = state,
         onExit = {},
         onRefreshStatus = {},
         onOpenShizuku = {},

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

@@ -205,8 +205,12 @@ class ContributionWizardViewModel @Inject constructor(
         when (mutableState.value.step) {
             WizardStep.INTRO -> if (mutableState.value.shizukuReady) transitionTo(WizardStep.DETAILS)
             WizardStep.DETAILS -> transitionTo(WizardStep.CAPTURE)
-            // Never advance while a capture is in flight — Review must reflect a settled session.
-            WizardStep.CAPTURE -> if (rawSession.observations.isNotEmpty() && captureJob?.isActive != true) buildReview()
+            // Never advance while a capture is in flight — Review must reflect a settled session. Two observations are
+            // the authoritative minimum: a diff needs something to diff against, so a single capture can only ever
+            // derive an empty matrix and would produce a report with no discovery data at all.
+            WizardStep.CAPTURE -> if (rawSession.observations.size >= MIN_MODES && captureJob?.isActive != true) {
+                buildReview()
+            }
             WizardStep.REVIEW -> buildDelivery()
             WizardStep.DELIVER -> Unit
         }
@@ -287,7 +291,9 @@ class ContributionWizardViewModel @Inject constructor(
     private fun diffCount(before: Map<SettingId, String>, after: Map<SettingId, String>): Int =
         (before.keys + after.keys).count { before[it] != after[it] }
 
-    private companion object {
-        val TAG = logTag("Contribution", "VM")
+    companion object {
+        /** A comparison needs at least two modes; one capture derives an empty matrix by construction. */
+        const val MIN_MODES = 2
+        private val TAG = logTag("Contribution", "VM")
     }
 }

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

@@ -310,6 +310,7 @@
     <string name="contribution_label_duplicate">You already captured a mode with that name.</string>
     <string name="contribution_status_no_change">That mode changed no settings — recorded anyway.</string>
     <string name="contribution_status_shizuku_required">Shizuku access is required to capture.</string>
+    <string name="contribution_capture_empty_error">Nothing could be read from the settings providers. Check that Shizuku is still running, then try again.</string>
     <string name="contribution_undo_last">Undo last</string>
     <string name="contribution_restart">Start over</string>
     <string name="contribution_modes_captured">%1$d mode(s) captured</string>
@@ -327,7 +328,10 @@
     <string name="contribution_review_hidden">Hidden</string>
     <string name="contribution_review_reveal">Reveal</string>
     <string name="contribution_review_include">Include in the public report</string>
-    <string name="contribution_review_empty">No settings changed across the captured modes.</string>
+    <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>
 
     <!-- Step: deliver -->
     <string name="contribution_deliver_title">Send it</string>
@@ -340,6 +344,7 @@
 
     <!-- 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/core/ContributionReportTest.kt

@@ -95,6 +95,42 @@ class ContributionReportTest {
         built.withheldRowCount shouldBe 0
     }
 
+    @Test
+    fun `an empty matrix reads as nothing changed, not as a contributor redaction`() {
+        val id = secure("charge_optimization_mode")
+        val built = report(RawWizardSession(listOf(obs("a", id to "1"), obs("b", id to "1"))))
+        built.changedRowCount shouldBe 0
+        built.withheldRowCount shouldBe 0
+        val text = formatContributionReport(built)
+        text shouldContain "no settings changed across the captured modes"
+        text shouldNotContain "approved for inclusion"
+        text shouldContain "changed_rows=0"
+    }
+
+    @Test
+    fun `a fully withheld matrix keeps the inclusion wording and leaks no key`() {
+        val id = secure("lock_screen_owner_info")
+        val built = report(RawWizardSession(listOf(obs("a", id to "SECRET-A"), obs("b", id to "SECRET-B"))))
+        built.changedRowCount shouldBe 1
+        built.withheldRowCount shouldBe 1
+        val text = formatContributionReport(built)
+        text shouldContain "(no settings approved for inclusion)"
+        text shouldContain "withheld_rows=1"
+        text shouldNotContain "lock_screen_owner_info"
+        text shouldNotContain "SECRET"
+    }
+
+    @Test
+    fun `report states what was measured`() {
+        val text = formatContributionReport(
+            report(RawWizardSession(listOf(obs("off", global("protect_battery") to "0"), obs("max", global("protect_battery") to "1")))),
+        )
+        text shouldContain "contribution_schema=2"
+        text shouldContain "captured_mode_count=2"
+        text shouldContain "scanned_namespaces=secure,global,system"
+        text shouldContain "changed_rows=1"
+    }
+
     @Test
     fun `multiline values are collapsed to one line`() {
         val id = secure("charge_optimization_mode")

app/src/test/java/eu/darken/amply/diagnostics/core/DefaultContributionRepositoryTest.kt

@@ -32,12 +32,13 @@ import org.robolectric.annotation.Config
 @Config(sdk = [34])
 class DefaultContributionRepositoryTest {
 
-    private class RecordingSource : SettingsSnapshotSource {
+    private class RecordingSource(private val values: Map<String, String> = mapOf("some_key" to "1")) :
+        SettingsSnapshotSource {
         val requested = mutableListOf<SettingNamespace>()
         override suspend fun status() = BackendStatus(available = true, granted = true, detail = "test".toCaString())
         override suspend fun snapshot(namespace: SettingNamespace): NamespaceSnapshot {
             requested += namespace
-            return NamespaceSnapshot.Success(emptyMap())
+            return NamespaceSnapshot.Success(values)
         }
     }
 
@@ -74,4 +75,14 @@ class DefaultContributionRepositoryTest {
         )
         source.requested shouldNotContain SettingNamespace.LINEAGE_SYSTEM
     }
+
+    @Test
+    fun `a capture that reads nothing at all fails instead of passing as an empty snapshot`() = runTest {
+        // A backend only reports Failure when the call threw; empty/unparsable output parses to an empty map. Three
+        // empty namespaces would otherwise diff to "nothing changed" and look like a valid unsupported-ROM finding.
+        val source = RecordingSource(values = emptyMap())
+        val repo = DefaultContributionRepository(ApplicationProvider.getApplicationContext(), source, registry)
+
+        repo.captureSnapshot().shouldBeInstanceOf<CaptureResult.Failure>()
+    }
 }