Bound the wedge diagnostics, so a wedged leg reports as a wedge (#122)

A wedge already reports itself correctly, and then the leg spends half an hour
not saying so. From #151, run 33071570911:

    12:31:29  > Task :app:connectedDebugAndroidTest
    12:48:44  wedged: yes -- gradle was killed after 1200s and never returned
    12:49:20  ::warning::E2E api37 WEDGED (api37)
              ... 36 minutes of nothing ...
    13:25:05  ##[error]The operation was canceled.        <- timeout-minutes: 60
    13:25:09  Terminate orphan process: qemu-system-x86_64-headless, adb, java, java

WEDGE_TIMEOUT=1200 fired exactly as designed. What overran was everything after
it: `capture_wedge` and `dump_diagnostics` probe the device with adb, and the
device they are probing has just finished proving it stopped answering.

WHY `|| true` DID NOT COVER THIS. Every probe carries one, and the file header
says why:

    # Every probe is guarded with `|| true`. A diagnostic must never be the thing
    # that turns a run red.

`|| true` guards a probe that exits non-zero. It does nothing about one that
never exits at all. The discipline was enforced for exit codes and not for time,
and this is the other half of it.

WHAT IT COSTS BEYOND THE 36 MINUTES. The job ends `cancelled` rather than failing
with the wedge's own status, so `gh pr checks` renders a failure with no cause and
the carefully-built `wedged:` row sits above half an hour of silence -- the one
mechanism built to explain a wedge is the least likely to be read. `kill
"$LOGCAT_PID"` never runs either, which is the orphaned adb and qemu above. And a
cancelled required check blocks merges: this one stopped a seven-PR stack.

THE FIX. An `adbq` wrapper -- `timeout -k 5s 20 adb "$@"` -- applied to all 18
probes in `capture_wedge`, `dump_diagnostics`, and the pre-run memory snapshot.
20s is far more than any of them needs on a healthy device and far less than any
costs on a dead one; `-k` because adb itself can ignore the first signal when its
server is wedged.

WHAT IS DELIBERATELY LEFT UNBOUNDED, and it is not everything else by accident:

  - The API 37 SystemUI disable machinery (`adb shell stop`/`start`, the
    `service check` loop). Functional, not diagnostic, and it already carries its
    own verify-and-retry -- see the comment block above it.
  - The backgrounded `adb logcat -v time` stream. It is meant to run for the whole
    leg; bounding it would truncate the log at 20 seconds.

Verified rather than assumed. Wrapper semantics, smoke-tested against a fake adb:
passthrough works, a non-zero exit is preserved (rc=7), a hang is killed at the
deadline (rc=124 after 2s), and `|| true` still swallows that -- so a bounded
probe cannot turn a run red either, which is the property the header demands.

shellcheck clean at the pinned digest over `git ls-files '*.sh'`, actionlint clean
at its pinned digest, `bash -n` clean.

One thing this does NOT do: it does not stop the wedge. #122 is still open for
that. It stops a wedge from being reported as a cancellation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

.github/scripts/e2e-run.sh

@@ -79,6 +79,21 @@ WEDGE_TIMEOUT=1200
 # package against `pm list packages -d`, and require a 45 s window with zero new aborts.
 # Three rounds, because one is not reliable and the failure is silent.
 # ---------------------------------------------------------------------------
+# Every device probe below is aimed at an emulator that has already failed, and on the wedge path
+# at one that has just finished proving it stopped answering. So each is bounded in time as well as
+# in exit status.
+#
+# `|| true` guards a probe that exits non-zero. It does nothing about one that never exits -- which
+# is how #122's wedge path spent 36 minutes after printing its own diagnosis, lost the job to the
+# 60-minute cap, and so reported `cancelled` instead of the wedge's own status. The header's rule
+# that "a diagnostic must never be the thing that turns a run red" was enforced for exit codes and
+# not for time; this is the other half of it.
+#
+# 20s is far more than any of these needs on a healthy device and far less than any of them costs
+# on a dead one. `-k` because adb itself can ignore the first signal when its server is wedged.
+ADB_PROBE_TIMEOUT=20
+adbq() { timeout -k 5s "$ADB_PROBE_TIMEOUT" adb "$@"; }
+
 count_aborts() { adb logcat -d -b crash 2> /dev/null | grep -c 'hasReadColorBufferDma'; }
 systemui_disabled() { adb shell pm list packages -d 2> /dev/null | grep -q 'com.android.systemui'; }
 
@@ -155,11 +170,11 @@ LOGCAT_PID=$!
 dump_diagnostics() {
   {
     echo "===== E2E api${LABEL} failure diagnostics -- $(date -u +%FT%TZ) ====="
-    echo "--- adb devices ---";                adb devices -l 2>&1 || true
-    echo "--- guest memory ---";               adb shell cat /proc/meminfo 2>&1 | grep -E 'MemTotal|MemAvailable|SwapTotal' || true
-    echo "--- guest storage ---";              adb shell df /data 2>&1 || true
-    echo "--- is the app even installed? ---"; adb shell pm list packages 2>&1 | grep -a libremedia || true
-    echo "--- native crashes ---";             adb logcat -d -b crash 2>&1 | tail -80 || true
+    echo "--- adb devices ---";                adbq devices -l 2>&1 || true
+    echo "--- guest memory ---";               adbq shell cat /proc/meminfo 2>&1 | grep -E 'MemTotal|MemAvailable|SwapTotal' || true
+    echo "--- guest storage ---";              adbq shell df /data 2>&1 || true
+    echo "--- is the app even installed? ---"; adbq shell pm list packages 2>&1 | grep -a libremedia || true
+    echo "--- native crashes ---";             adbq logcat -d -b crash 2>&1 | tail -80 || true
     echo "--- runner: kvm ---";                ls -l /dev/kvm 2>&1 || true
     echo "--- runner: memory ---";             free -h 2>&1 || true
     echo "--- runner: disk ---";               df -h 2>&1 || true
@@ -167,9 +182,9 @@ dump_diagnostics() {
 
   # Also to the step log, so the common case needs no artifact download.
   echo "----- FAILURE SUMMARY (api${LABEL}) -----"
-  adb shell cat /proc/meminfo 2>&1 | grep -E 'MemTotal|MemAvailable' || true
+  adbq shell cat /proc/meminfo 2>&1 | grep -E 'MemTotal|MemAvailable' || true
   echo "--- native crashes (tail 60) ---"
-  adb logcat -d -b crash 2>&1 | tail -60 || true
+  adbq logcat -d -b crash 2>&1 | tail -60 || true
 }
 
 capture_wedge() {
@@ -182,35 +197,39 @@ capture_wedge() {
     echo "--- running/last instrumented test (logcat TestRunner) ---"
     grep -a TestRunner "$LOGCAT_LOG" 2>/dev/null | tail -25 || true
     echo "--- boot state ---"
-    adb shell getprop sys.boot_completed 2>&1 || true
+    adbq shell getprop sys.boot_completed 2>&1 || true
     echo "--- are the binder services published? ---"
     for svc in input window activity media.player; do
-      echo "  service check $svc:"; adb shell service check "$svc" 2>&1 || true
+      echo "  service check $svc:"; adbq shell service check "$svc" 2>&1 || true
     done
-    APP_PID="$(adb shell pidof "$APP_ID" 2>/dev/null | tr -d '\r')" || true
-    TEST_PID="$(adb shell pidof "$TEST_ID" 2>/dev/null | tr -d '\r')" || true
+    APP_PID="$(adbq shell pidof "$APP_ID" 2>/dev/null | tr -d '\r')" || true
+    TEST_PID="$(adbq shell pidof "$TEST_ID" 2>/dev/null | tr -d '\r')" || true
     echo "--- pids --- app: ${APP_PID:-<none>}  test: ${TEST_PID:-<none>}"
     # SIGQUIT makes ART dump every thread's stack to logcat and /data/anr. This is what
     # distinguishes a deadlocked test from a stuck native encode from a dead device.
     echo "--- SIGQUIT thread dumps ---"
     for pid in $APP_PID $TEST_PID; do
-      [ -n "$pid" ] && adb shell kill -3 "$pid" 2>&1 || true
+      [ -n "$pid" ] && adbq shell kill -3 "$pid" 2>&1 || true
     done
     sleep 5
     echo "--- /data/anr/* ---"
-    adb shell 'cat /data/anr/* 2>/dev/null' 2>&1 || true
-    echo "--- dumpsys activity ---"; adb shell dumpsys activity 2>&1 || true
-    echo "--- dumpsys window ---";   adb shell dumpsys window 2>&1 || true
+    adbq shell 'cat /data/anr/* 2>/dev/null' 2>&1 || true
+    echo "--- dumpsys activity ---"; adbq shell dumpsys activity 2>&1 || true
+    echo "--- dumpsys window ---";   adbq shell dumpsys window 2>&1 || true
     # FFmpeg and Media3 both run through MediaCodec; a wedged transcode shows up here.
-    echo "--- dumpsys media.player ---"; adb shell dumpsys media.player 2>&1 || true
+    echo "--- dumpsys media.player ---"; adbq shell dumpsys media.player 2>&1 || true
     echo "--- logcat -d (tail 400, includes the SIGQUIT dump) ---"
-    adb logcat -d 2>&1 | tail -400 || true
+    adbq logcat -d 2>&1 | tail -400 || true
   } >> "$WEDGE_LOG" 2>&1 || true
   echo "::warning::E2E api${LABEL} WEDGED ($1) -- see the wedge-diagnostics-api${LABEL} artifact"
 }
 
 echo "::group::E2E api${LABEL}"
-adb shell cat /proc/meminfo 2>&1 | grep -E 'MemTotal|MemAvailable|SwapTotal' || true
+# Bounded like the probes in the two diagnostic functions, and for the same reason. This one runs
+# against a freshly booted emulator rather than a wedged one, so it is the least likely of them to
+# hang -- but it is still a `|| true` diagnostic, and the rule this file now states is that a
+# diagnostic must never be the thing that ends the leg.
+adbq shell cat /proc/meminfo 2>&1 | grep -E 'MemTotal|MemAvailable|SwapTotal' || true
 
 status=0
 # -k 30s SIGKILLs a gradle client that ignores SIGTERM. The wrapper covers ONLY the foreground

CLAUDE.md

@@ -130,9 +130,9 @@ install for code that can never run — and on API 37 the full APK does not fit
 - The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the
   decision layer, where one branch is one documented user-visible outcome and the metric counts
   answers rather than complexity. Every other rule still applies there.
-- **Coverage is reported, not gated** — **87.1% of lines (2025/2324), 69.1% of branches
-  (974/1410)**, measured 2026-08-27 with `./gradlew :app:jacocoTestReport`, against 502 JVM tests
-  in 71 classes.
+- **Coverage is reported, not gated** — **88.9% of lines (2087/2348), 75.4% of branches
+  (1011/1340)**, measured 2026-08-29 with `./gradlew :app:jacocoTestReport`, against 546 JVM tests
+  in 76 classes.
 
   **Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.**
   Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo
@@ -159,6 +159,21 @@ install for code that can never run — and on API 37 the full APK does not fit
   chooses a branch the suite had never taken. Line coverage barely notices; branch coverage is the
   whole point.
 
+  Then #153's five children on 2026-08-29 — 87.1% -> 88.9% line, 69.1% -> **75.4%** branch, 502 ->
+  546 tests.
+
+  **That last branch figure moved for two reasons, and only one of them is new tests.** The
+  numerator rose 974 -> 1011; the denominator *fell* 1410 -> 1340. Both are the seam work. Pulling
+  a `when` out of a lambda inside a `collect` deletes the coroutine state machine's synthesized
+  branches around it, and what is left is a plain function whose branches a test can choose:
+  `ConversionViewModel$observe$1$1` went from carrying the whole mapping to 6 branches, while the
+  extracted `ConversionViewModelKt` covers 41 of 42 and `JoinViewModelKt` 38 of 39. So a seam is
+  worth more than the tests it enables — it also stops the measurement counting scaffolding.
+
+  Be careful quoting a branch move on its own for that reason. A percentage that rises because the
+  denominator shrank is not the same claim as one that rises because more branches are tested, and
+  this entry has a history of explaining its own numbers wrongly.
+
   And **re-measure before quoting**: this entry was once written quoting 81.4%, measured four hours
   earlier, and was already three points stale by the time it was ready to merge.
 - **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
@@ -182,6 +197,32 @@ install for code that can never run — and on API 37 the full APK does not fit
 
 - `kotlin.code.style=official`. Gradle stays Kotlin DSL.
 
+- **A stacked PR does not merge with `gh pr merge`, and `MERGED` is not proof it reached `main`.**
+  Two separate traps, both measured on 2026-08-27 while landing #144-#151.
+
+  `gh pr merge` uses the GraphQL mutation, which refuses a stacked PR outright: *"This pull request
+  is part of a stack and must be merged using the asynchronous merge REST API."* So does
+  `PUT .../pulls/{n}/merge`. The one that works is
+  `gh api -X PUT repos/OWNER/REPO/pulls/N/merge-async -f merge_method=merge`, which returns a uuid
+  to poll at `.../merge-async/{uuid}` until `status` is `merged` or `failed`.
+
+  **The second trap is worse, because nothing looks wrong.** GitHub retargets a stacked PR's base
+  to `main` when the PR below it merges, but *asynchronously*. Merge a stack faster than that
+  settles — five PRs about thirty seconds apart, in the case that found this — and each one merges
+  into its own base branch, which has itself already been merged and left behind. Every call
+  returns `status: merged` and every one is true. `gh pr list --state open` comes back empty, every
+  PR shows `MERGED`, and **none of the content is on `main`**.
+
+  What caught it was a coverage re-measure reading two points lower than the same tree had measured
+  an hour earlier; a fresh `git pull` changed nothing, which is what turned it into a question.
+  `git merge-base --is-ancestor <merge-sha> origin/main` answers it in one line. Do that after
+  merging a stack, or merge one at a time and re-read `baseRefName` between. #160 is what the
+  recovery cost.
+
+  The auto-retarget belongs to the stacking feature specifically. A PR opened with a plain
+  `gh pr create --base some-branch` does **not** retarget when that branch merges — it is left
+  pointing at a dead base and has to be moved by hand.
+
 - **File one-off issues with `tools/github/file-issue.sh`, not `gh issue create`.** `gh issue
   create` does not touch the project board, so the issue exists, carries its labels, and is
   invisible in the Kanban — indistinguishable from never having been filed. Measured 2026-08-24:

app/src/test/java/org/libremediaconverter/convert/SettingsEditsTest.kt

@@ -0,0 +1,197 @@
+package org.libremediaconverter.convert
+
+import android.app.Application
+import androidx.media3.common.util.UnstableApi
+import androidx.work.Data
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotEquals
+import org.junit.Assert.assertNull
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.libremediaconverter.model.AudioCodec
+import org.libremediaconverter.model.Container
+import org.libremediaconverter.model.EnginePreference
+import org.libremediaconverter.model.OutputFormat
+import org.libremediaconverter.model.QualityTier
+import org.libremediaconverter.model.VideoCodec
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.RuntimeEnvironment
+
+/**
+ * The seven one-line edits the settings sheet makes, and what each one leaves alone.
+ *
+ * ## Why these needed a file of their own
+ *
+ * `setPreset` was covered. The six beside it — `setContainer`, `setVideoCodec`, `setAudioCodec`,
+ * `applySuggestion`, `setQuality`, `setEnginePreference` — and `cancel()` had **no coverage at
+ * all**, which is the tell: they are reachable from the JVM suite by exactly the route `setPreset`
+ * already takes, and nothing had asked.
+ *
+ * ## What is actually being asserted
+ *
+ * Not "the setter sets something". Each of these copies into a nested `OutputSpec`, so the failure
+ * worth catching is **a setter that writes the right value into the wrong field, or that rebuilds
+ * the spec and silently discards the other two**. So every test here asserts the field it changed
+ * *and* that the rest of the spec survived — a `setContainer` implemented as
+ * `it.copy(spec = OutputFormat.MP4_H265.spec.copy(container = container))` would pass a test that
+ * only checked the container.
+ *
+ * `ConverterScreenContentTest` cannot cover this: it builds `ConverterActions` itself and never
+ * touches the ViewModel. That the *screen* calls these is #156's, and neither implies the other.
+ */
+@UnstableApi
+@RunWith(RobolectricTestRunner::class)
+class SettingsEditsTest {
+
+    private lateinit var app: Application
+    private lateinit var viewModel: ConversionViewModel
+
+    @Before
+    fun setUp() {
+        app = RuntimeEnvironment.getApplication()
+        installTestWorkManager(app, Data.EMPTY)
+        viewModel = ConversionViewModel(app)
+    }
+
+    @After
+    fun tearDown() {
+        ConversionDependencies.reset()
+    }
+
+    @Test
+    fun `choosing a preset replaces the whole spec`() {
+        viewModel.setPreset(OutputFormat.WEBM_VP9)
+
+        assertEquals(OutputFormat.WEBM_VP9.spec, viewModel.settings.value.spec)
+    }
+
+    @Test
+    fun `changing the container leaves both codecs alone`() {
+        // Moved off the default spec first, and that is load-bearing rather than tidiness. The
+        // default IS `OutputFormat.MP4_H265.spec`, so a `setContainer` that rebuilt the spec from
+        // that preset instead of from the current one produced an identical answer and the
+        // mutation went green. Editing the codecs away from the default first is what makes
+        // "the other two survived" an assertion rather than a coincidence.
+        viewModel.setPreset(OutputFormat.WEBM_VP9)
+        val before = viewModel.settings.value.spec
+
+        viewModel.setContainer(Container.MKV)
+
+        val after = viewModel.settings.value.spec
+        assertEquals(Container.MKV, after.container)
+        assertEquals("the video codec is not the container's to change", before.videoCodec, after.videoCodec)
+        assertEquals("the audio codec is not the container's to change", before.audioCodec, after.audioCodec)
+    }
+
+    @Test
+    fun `changing the video codec leaves the container and the audio codec alone`() {
+        // The transposition this guards against is real: setVideoCodec and setAudioCodec take
+        // different enum types, but a copy(...) naming the wrong field compiles wherever the types
+        // happen to line up, and the picker would silently set the other one.
+        val before = viewModel.settings.value.spec
+
+        viewModel.setVideoCodec(VideoCodec.VP9)
+
+        val after = viewModel.settings.value.spec
+        assertEquals(VideoCodec.VP9, after.videoCodec)
+        assertEquals(before.container, after.container)
+        assertEquals(before.audioCodec, after.audioCodec)
+    }
+
+    @Test
+    fun `changing the audio codec leaves the container and the video codec alone`() {
+        val before = viewModel.settings.value.spec
+
+        viewModel.setAudioCodec(AudioCodec.OPUS)
+
+        val after = viewModel.settings.value.spec
+        assertEquals(AudioCodec.OPUS, after.audioCodec)
+        assertEquals(before.container, after.container)
+        assertEquals(before.videoCodec, after.videoCodec)
+    }
+
+    @Test
+    fun `applying a suggestion replaces the spec without disturbing quality or engine`() {
+        // A suggestion comes from ContainerCapabilities when the current spec is invalid, so it is
+        // a whole spec by construction. What it must not do is reset the two settings beside it.
+        viewModel.setQuality(QualityTier.BEST)
+        viewModel.setEnginePreference(EnginePreference.FORCE_SOFTWARE)
+
+        viewModel.applySuggestion(OutputFormat.MKV_H264.spec)
+
+        val settings = viewModel.settings.value
+        assertEquals(OutputFormat.MKV_H264.spec, settings.spec)
+        assertEquals(QualityTier.BEST, settings.quality)
+        assertEquals(EnginePreference.FORCE_SOFTWARE, settings.enginePreference)
+    }
+
+    @Test
+    fun `changing the quality leaves the spec and the engine preference alone`() {
+        // Both neighbours are moved off their defaults first. Asserting against AUTO -- which is
+        // what `ConversionSettings` starts with -- let a `setQuality` that also reset the engine
+        // preference to AUTO pass, because the reset and the survival looked identical.
+        viewModel.setPreset(OutputFormat.WEBM_VP9)
+        viewModel.setEnginePreference(EnginePreference.FORCE_SOFTWARE)
+        val before = viewModel.settings.value.spec
+
+        viewModel.setQuality(QualityTier.BEST)
+
+        val settings = viewModel.settings.value
+        assertEquals(QualityTier.BEST, settings.quality)
+        assertEquals(before, settings.spec)
+        assertEquals(
+            "quality is not the engine preference's to change",
+            EnginePreference.FORCE_SOFTWARE,
+            settings.enginePreference,
+        )
+    }
+
+    @Test
+    fun `changing the engine preference leaves the spec and the quality alone`() {
+        // Off the defaults for the same reason as the test above: QualityTier.FAST is the starting
+        // value, so asserting it here would have been satisfied by a reset as readily as by a
+        // survival.
+        viewModel.setPreset(OutputFormat.WEBM_VP9)
+        viewModel.setQuality(QualityTier.BEST)
+        val before = viewModel.settings.value.spec
+
+        viewModel.setEnginePreference(EnginePreference.FORCE_SOFTWARE)
+
+        val settings = viewModel.settings.value
+        assertEquals(EnginePreference.FORCE_SOFTWARE, settings.enginePreference)
+        assertEquals(before, settings.spec)
+        assertEquals(
+            "the engine preference is not the quality's to change",
+            QualityTier.BEST,
+            settings.quality,
+        )
+    }
+
+    @Test
+    fun `editing past every preset leaves no matching preset`() {
+        // `matchingPreset` is what the settings sheet reads to decide whether to show a preset as
+        // selected or to say "Custom". Editing one field of a preset must drop it out of the list
+        // rather than leaving the old one highlighted.
+        viewModel.setPreset(OutputFormat.MP4_H265)
+        assertEquals(OutputFormat.MP4_H265, viewModel.settings.value.matchingPreset)
+
+        viewModel.setAudioCodec(AudioCodec.FLAC)
+
+        assertNull(
+            "an edited spec is no longer any preset, and the sheet says Custom",
+            viewModel.settings.value.matchingPreset,
+        )
+        assertNotEquals(OutputFormat.MP4_H265.spec, viewModel.settings.value.spec)
+    }
+
+    @Test
+    fun `cancelling with no active job does nothing rather than throwing`() {
+        // `activeWorkId?.let(...)` -- the null side. A user can reach Cancel through a state that
+        // has already finished, and taking the app down for it would be worse than doing nothing.
+        viewModel.cancel()
+
+        assertEquals(ConversionState.Idle, viewModel.state.value)
+    }
+}